Remove ActiveSupport String#exclude?

[dduugg]

• 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 typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

As a continuation of #16184 this removes String#exclude?. (I've refactored all exclude? calls to use the inverse method, since Enumerable#exclude? is also via ActiveSupport.)

[Bo98]

There's some uses in other taps that need updating first: https://github.com/search?q=org%3AHomebrew+exclude%3F+-repo%3AHomebrew%2Fbrew+-repo%3AHomebrew%2Fbrew-ghsa-h7hq-48hm-3mfj+language%3ARuby&type=code&l=Ruby.

However, due to the Rails/NegateInclude cop being enforced on formulae, we will likely need some longer term backwards compatibility here since we've forced it upon third party taps: https://github.com/search?q=%22%3C+Formula%22+%22exclude%3F%22+-is%3Afork+-org%3AHomebrew&type=code

[dduugg]

There's some uses in other taps that need updating first: https://github.com/search?q=org%3AHomebrew+exclude%3F+-repo%3AHomebrew%2Fbrew+-repo%3AHomebrew%2Fbrew-ghsa-h7hq-48hm-3mfj+language%3ARuby&type=code&l=Ruby.

However, due to the Rails/NegateInclude cop being enforced on formulae, we will likely need some longer term backwards compatibility here since we've forced it upon third party taps: https://github.com/search?q=%22%3C+Formula%22+%22exclude%3F%22+-is%3Afork+-org%3AHomebrew&type=code

Thanks for the heads up. I've restored the monkey-patch and left a comment in global.rb for the core extensions that I know are required for downstream compatibility (there may be others). I've kept the removal of the rubocop that enforces its use, and the converted code within this repo. LMK what you think.

[Bo98]

Thanks for the heads up. I've restored the monkey-patch and left a comment in global.rb for the core extensions that I know are required for downstream compatibility (there may be others). I've kept the removal of the rubocop that enforces its use, and the converted code within this repo. LMK what you think.

I suppose it depends what the plan is. If the plan is to remove ActiveSupport entirely, it's probably worth just carrying the monkey-patch ourselves. The implementation sounds like it would be extremely trivial (without looking, it's probably just !include?(arg), so I could write that without needing to reference upstream and its license).

As to whether that means keeping the monkey-patch indefinitely or to deprecate, I'll defer to what others like @MikeMcQuaid prefer as I have no particularly strong opinion. I personally avoid certain ActiveSupportisms (e.g. present?) in favour of better typing control (e.g. accepting both nil and an empty array often doesn't make sense), but beyond that I don't feel strongly.

[dduugg]

Thanks for the heads up. I've restored the monkey-patch and left a comment in global.rb for the core extensions that I know are required for downstream compatibility (there may be others). I've kept the removal of the rubocop that enforces its use, and the converted code within this repo. LMK what you think.

I suppose it depends what the plan is. If the plan is to remove ActiveSupport entirely, it's probably worth just carrying the monkey-patch ourselves. The implementation sounds like it would be extremely trivial (without looking, it's probably just !include?(arg), so I could write that without needing to reference upstream and its license).

As to whether that means keeping the monkey-patch indefinitely or to deprecate, I'll defer to what others like @MikeMcQuaid prefer as I have no particularly strong opinion. I personally avoid certain ActiveSupportisms (e.g. present?) in favour of better typing control (e.g. accepting both nil and an empty array often doesn't make sense), but beyond that I don't feel strongly.

I've been chipping away at this as I work on the sorbet stuff, but it probably makes sense at this point to actually have a ticket to track what we ultimately do with ActiveSupport: #16190

Feel free to comment there if you have opinions. If we want to move forward with considering this, I can then make a table of what we're using, what has downstream dependencies, and what we to do (i.e. convert call sites or implement directly).

(As always, I'm happy to have this PR closed out if it seems to be jumping the gun)

[MikeMcQuaid]

Thanks, have commented more generally on #16190.

I suppose it depends what the plan is. If the plan is to remove ActiveSupport entirely, it's probably worth just carrying the monkey-patch ourselves. The implementation sounds like it would be extremely trivial (without looking, it's probably just !include?(arg), so I could write that without needing to reference upstream and its license).

Yes, an implementation like that seems ideal. Where we can keep around things like exclude?, blank?, present? etc. with a one-liner for identical (or near-identical) functionality: we should.

As to whether that means keeping the monkey-patch indefinitely or to deprecate, I'll defer to what others like @MikeMcQuaid prefer as I have no particularly strong opinion. I personally avoid certain ActiveSupportisms (e.g. present?) in favour of better typing control (e.g. accepting both nil and an empty array often doesn't make sense), but beyond that I don't feel strongly.

I like present?, presence and blank? because I think they often allow you to be more expressive and avoid doing nil-checks all over the place for e.g. empty-or-nil arrays.

The ActiveSupport implementations are fairly involved; I'd propose we create our own simple implementations here instead.

[Bo98]

I like present?, presence and blank? because I think they often allow you to be more expressive and avoid doing nil-checks all over the place for e.g. empty-or-nil arrays.

Yeah for sure, I didn't mean to suggest never. I just don't use it as liberally as before and think about it when doing so as if I write a function that takes in an array, I would just make the type signature disallow nil to begin with. Allowing both nil and empty can be a sign of poor type control, and did make Sorbet adoption a little harder in places.

That doesn't mean that's always the case though. There's a reasonable notion of, for example, a CLI arg that's not supplied (nil) and an empty arg (""). Being a CLI tool, that applies a lot to us.

I agree with the approach here: we can definitely have our own implementation of the presence functions etc and remove ActiveSupport entirely. General rule of thumb I'd say is: if it's used in Homebrew/core, consider it public API as it is supposed to only be public API there (which people copy as inspiration or via brew extract). Public API needs either indefinite support or a defined deprecation path (we'll be making a new minor release perhaps by the end of the year that could be made use of).

[dduugg]

Pausing, pending scheduling in #16190