Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unlimited terms after ellipsis #6843

Merged
merged 11 commits into from Mar 18, 2019
Merged

Unlimited terms after ellipsis #6843

merged 11 commits into from Mar 18, 2019

Conversation

marcandre
Copy link
Contributor

This makes it possible to have node patterns with as many terms as wanted after an ... or an $....

The initial commits are refactoring the code to make small improvements, factorize common code. Overall code is shorter.

Fixes #6840. Should help for #6841 too.

@marcandre marcandre changed the title Ellipsis Unlimited terms after ellipsis Mar 17, 2019
Copy link
Collaborator

@Drenmi Drenmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great improvement! 🙇 I'm okay with the changes. Please squash your commits, and I'll get this merged. 🙂

@marcandre
Copy link
Contributor Author

@Drenmi Thanks. With all due respect, I won't squash my commits and hope you don't either.
Please read this as for why. Note that 48016a5ffce1359 contains a test that is later removed, so squashing would actually lose information.

@koic
Copy link
Member

koic commented Mar 18, 2019

First of all, thank you for your work. However, we maintainers do not expect to be committed to the progress of work. I think there is a policy for each repository therefore please squash in this repository. Thank you.

@marcandre
Copy link
Contributor Author

@koic
Copy link
Member

koic commented Mar 18, 2019

I know there are many more discussions about squash besides your blog entry, but I hoped to prioritize repository contribution guides that the maintainers wants.

Squash related commits together.

https://github.com/rubocop-hq/rubocop/blob/v0.65.0/CONTRIBUTING.md#pull-requests

I don't want to spend a long time on git discussions. It will settle on who will do squash (or squash merge). So I hope to be able to merge very nice improvements quickly.

@marcandre
Copy link
Contributor Author

@koic Unless you can provide a good rationale (note that "because it's in the contributing file" is not a rationale), please don’t ask me to squash the commits of my pull requests.

Don’t ask me because, with all due respect to the amazing work from committers, I won’t do it.

Unless I’ve made an actual mistake, if my pull request has 5 commits, it is because each of them is independent and I feel they should remain so.

If they really want to, project committers can manually squash and merge the commits, wait for someone else to make a PR with the commits squashed or even reject the PR. I hope they gain more by doing so than alienating competent committers.

Call it lunacy or pride, but I just won’t squash my commits. Hopefully committers won’t take offense at that as I’ll do my best not to take offense at their suggestion that I didn’t segment my commits correctly.

@Drenmi
Copy link
Collaborator

Drenmi commented Mar 18, 2019

Unless you can provide a good rationale [...]

I will need to ask you to please not invite bike shedding. 😅 If you have decided you don't want to squash commits, then that's okay. Your contributions are still welcome.

I am not entirely opposed to keeping some commits for posterity, but for them to be at all useful (at least to me as a reviewer) they need to come with some meaningful annotation. Loading context is expensive, and putting some of it in the commit messages for the benefit of the reader has a lot of value. 🙂

We might also squash some of the commits in some cases which, if I understand your blog post correctly, is okay with you?

@marcandre
Copy link
Contributor Author

marcandre commented Mar 18, 2019

I am not entirely opposed to keeping some commits for posterity, but for them to be at all useful (at least to me as a reviewer) they need to come with some meaningful annotation.

I've attempted to make the commit messages more meaningful. Please read an implicit "This is a net improvement" in all the commit messages 😸. I've also added an entry in the changelog.

Would you be kind enough to tell me which commit(s) may not be easy to review, may not be seen as an improvement, or doesn't have a meaningful comment, and also which commits aren't independent and should be squashed together? Or should I make separate PRs?

We might also squash some of the commits in some cases which, if I understand your blog post correctly, is okay with you?

I appreciate that you read my post. I meant that committers can do it, but should not. I imagine you will have to weigh the perceived gain in doing so with the possibility of alienating contributors like me. I didn't mean I would be happy if you were to exercise that right; I would not.

@koic koic merged commit 7c82792 into rubocop:master Mar 18, 2019
@koic
Copy link
Member

koic commented Mar 18, 2019

I understand that it is against your will, but I squash merged it while grateful for your improvement. Thank you again for taking the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants