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

pr-automerge: pluralize message only when needed #8362

Merged
merged 1 commit into from Aug 18, 2020

Conversation

dtrodrigues
Copy link
Member

@dtrodrigues dtrodrigues commented Aug 16, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Don't pluralize message when there is only one matching pull request.

@@ -49,7 +49,7 @@ def pr_automerge
return
end

ohai "#{prs.size} matching pull requests:"
ohai "#{prs.size} matching pull #{"request".pluralize(prs.size)}:"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ohai "#{prs.size} matching pull #{"request".pluralize(prs.size)}:"
ohai "#{prs.count} matching pull #{"request".pluralize(prs.count)}:"

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't mind changing, but why is count preferred to size?

Copy link
Member

Choose a reason for hiding this comment

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

It reads nicer. 😉

Copy link
Member

Choose a reason for hiding this comment

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

It reads nicer. 😉

It sounds like a personal preference that we shouldn't be casting on other maintainers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not change existing code if there isn't a good reason, so I'll leave it as is.

Copy link
Member

Choose a reason for hiding this comment

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

It's not really a personal preference: You can count multiple PRs, but you can't measure their size.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to suggest personal preference. I agree this is easier to read. It'd fine to me either way so I'm approving but I'd rather see "comment and approve" than maintainers such as @reitermarkus being encouraged to not leave these sort of (useful) comments.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to suggest personal preference.

It is sure fine to make suggestions based on personal preferences, but they should be explained (initial comment had no accompanying explanation) and provide at least some reasoning. I agree with the explanation provided later that one can count PRs but can't measure their size but I disagree with "it reads nicer".

@@ -49,7 +49,7 @@ def pr_automerge
return
end

ohai "#{prs.size} matching pull requests:"
ohai "#{prs.size} matching pull #{"request".pluralize(prs.size)}:"
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to suggest personal preference. I agree this is easier to read. It'd fine to me either way so I'm approving but I'd rather see "comment and approve" than maintainers such as @reitermarkus being encouraged to not leave these sort of (useful) comments.

@dtrodrigues
Copy link
Member Author

Prior to the more in-depth explanation from @reitermarkus, it wasn't clear to me why count was better and it seemed preferable to stay with the code that existed prior to the PR if there wasn't a strong reason to change it. Given the more detailed explanation, I agree that count does flow more nicely in the code, so I've made the change.

@dtrodrigues dtrodrigues merged commit 7241963 into Homebrew:master Aug 18, 2020
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 16, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants