RuboCop: MacOS

[reitermarkus]

• 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 tests with your changes locally?

[reitermarkus]

It's probably better to simply exclude this file (as I already did) from the ConstantName cop instead of this workaround. Opinions?

[MikeMcQuaid]

Agreed.

[MikeMcQuaid]

Although I might use an inline exclude?

[MikeMcQuaid]

(just so we can start killing the Exclude lines in the Rubocop files in favour of inline comments and avoid a similar problem in the same file)

[reitermarkus]

Changed it to an inline exclude.

[MikeMcQuaid]

Note to other reviewers: https://github.com/Homebrew/brew/pull/1125/files?w=1 makes this much easier.

[zmwangx]

Excuse me, have you read every change and made sure they're really equivalent? Because #1050 for instance (specifically 52ff988#diff-791b01b40fb03181802be9c2315258b2R153) seems to causing us audit errors which have at least been seen by three people so far:

$ brew audit --new-formula --display-cop-names --display-filename
Error: type mismatch: String given
Please report this bug:
    https://git.io/brew-troubleshooting
/usr/local/Homebrew/Library/Homebrew/cmd/search.rb:153:in `=~'
/usr/local/Homebrew/Library/Homebrew/cmd/search.rb:153:in `block in search_tap'
/usr/local/Homebrew/Library/Homebrew/cmd/search.rb:153:in `select'
/usr/local/Homebrew/Library/Homebrew/cmd/search.rb:153:in `search_tap'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/audit.rb:346:in `block (2 levels) in audit_formula_name'

If your answer to my question is no, can we stop these pointless, poorly-reviewed changes already?

[jawshooah]

If your answer to my question is no, can we stop these pointless, poorly-reviewed changes already?

To be fair, the parameter causing the type mismatch is called rx, which most people would assume to be a Regexp, no? If CI didn't break, I see no reason why a reviewer wouldn't glance over that and think it looked fine. If anything, the break simply exposed either a poorly named parameter or an improper use of the method, which would ideally be caught in tests to begin with.

[zmwangx]

called rx, which most people would assume to be a Regexp, no?

That's the right way to reason about code, yes.

If CI didn't break, I see no reason why a reviewer wouldn't glance over that and think it looked fine.

And I see no reason to make the change just to conform to some arbitrary style guide, especially when the change is made by a program that's not guaranteed to be correct, and the replaced version is not obviously equivalent (I'm not sure, not a Ruby programmer myself and don't know about the intricacies).

If nothing breaks, sure. But if things do break, the one who made the change should be held accountable (which isn't a big deal, because everyone makes mistakes), and we should stop and think about whether these mass changes are truly justified. The way I see it, these changes just makes git-blame useless while hardly making the code a bit more readable.

[MikeMcQuaid]

Excuse me, have you read every change and made sure they're really equivalent? Because #1050 for instance (specifically 52ff988#diff-791b01b40fb03181802be9c2315258b2R153) seems to causing us audit errors which have at least been seen by three people so far

One or more of those people should have created an issue. I looked periodically for new issues and did not get one. If fixes need to be made in the short-term (e.g. when other maintainers are asleep) then it's worth creating a PR to fix them, even if you didn't introduce the bug.

If your answer to my question is no, can we stop these pointless, poorly-reviewed changes already?

You may not agree with the point but they are not pointless. They are investment in a consistent style which makes it easier to automatically review contributed PRs and lets people know what our style is rather than it being arbitrary. If they are poorly-reviewed: everyone has contributed to that. You are more than welcome to ask for a time period (e.g. 24h, 48h) to give you personally more time to better review these changes.

If CI didn't break, I see no reason why a reviewer wouldn't glance over that and think it looked fine. If anything, the break simply exposed either a poorly named parameter or an improper use of the method, which would ideally be caught in tests to begin with.

I agree. We should add a failing test to handle this case. This is partly a process failure but also partly a CI coverage failure.

especially when the change is made by a program that's not guaranteed to be correct

It's not made by a program, it's made by a person and people make mistakes (you too, you know). We've all made mistakes in the last week and I'd like to you to think how you'd feel if your mistakes produced a similar reaction.

But if things do break, the one who made the change should be held accountable (which isn't a big deal, because everyone makes mistakes)

They are accountable but hitting on your hands waiting for a fix from them (particularly considering they are in a timezone when they are likely to be asleep) is your decision. A 24h turnaround on such a change is a reasonable expectation.

[zmwangx]

I agree. We should add a failing test to handle this case. This is partly a process failure but also partly a CI coverage failure.

People may disagree, but to me, "all tests pass => everything is good" is totally unrealistic, however good your tests are and coverage is. Plus the test directory is itself being fixed up by these RuboCop PRs too.

It's not made by a program, it's made by a person and people make mistakes (you too, you know). We've all made mistakes in the last week and I'd like to you to think how you'd feel if your mistakes produced a similar reaction.

I take full responsibility for any mistake I make, and if I deserve "a similar reaction" (pretty sure I did for one case) I'll take it. The reaction is just a cold wake up call; it's not ad hominem or anything.

[reitermarkus]

Excuse me, have you read every change and made sure they're really equivalent?

Yes. SinceOS::Mac is a module, module_function does not break anything as it simply replaces instance methods (which don't make sense for a module) with class methods.

[MikeMcQuaid]

People may disagree, but to me, "all tests pass => everything is good" is totally unrealistic, however good your tests are and coverage is.

It may be unrealistic but we should strive for a test suite where that's the case. We should aim to catch everything with CI that we can.

I take full responsibility for any mistake I make, and if I deserve "a similar reaction" (pretty sure I did for one case) I'll take it. The reaction is just a cold wake up call; it's not ad hominem or anything.

Wake up call is appropriate language as it was used while @reitermarkus and I were both asleep. You are entitled to take full responsibility and entitled to ask people to react to you in a certain way but this project has a Code of Conduct that aims to similarly spell out expectations of everyone's communications and, to me, that excludes cold wake up calls when the same information can be explained in a more diplomatic fashion.

It's also perhaps worth noting that when diplomacy is lacking it takes effort to deal with the consequences. This morning I had time to either try to smooth some of this over with people (including myself) who were unimpressed with the reaction or to try to fix the issue. As there was no issue filed with reproducible instructions on how to reproduce this I was unable to get a fix out before I had to rush out to a prearranged commitment.

To be clear, I'm not without fault either. I should have provided more in-depth review and encouraged waiting a bit longer for approval and holding off doing so until I could be around to clean up mistakes that may result. That's something I'll be doing more of going forward.

[zmwangx]

@whomeverconcerned Sorry, I'm too tired to argue and don't want to spend time on things that I (and probably most other team members too, judging from the lack of reviewers) don't care about, unless something actually breaks or if I happen to notice something too outrageous for me (which is highly unlikely because whenever I see such a PR the first and last thing I do is click unsubscribe). So proceed however you want and sorry for the wasted time and energy.

I have voiced my concerns about the necessity and legitimacy of this whole series of contributions and its impact on the whole team without explicit consensus, but the concerns have been dismissed as "exposing other problems" or "shut up if you won't provide or continue to provide anything specific". What else can I say?

By the way, reread my old comments if you will, when you calm down and feel less attacked.

This is my last comment on this series of issues (including those that are coming).

[MikeMcQuaid]

I've given this a reasonable review and we've just cut a new tag (1.0.5) so I'm going to merge this. I'm going to make sure in future I only merge style changes like this well before tagging.