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

various: use numeric Match addressing in strategy blocks #196546

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

samford
Copy link
Member

@samford samford commented Nov 3, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This updates strategy blocks in livecheck blocks to use numeric Match addressing (e.g., match[0] instead of match.first), as it's the prevailing standard and is less verbose. I've already done the same thing in homebrew/cask, so this brings homebrew/core in line.

In the process, this reworks the http_load strategy block to use a similar pattern to other formulae. The first capture group in the regex isn't optional, so it's guaranteed to exist in this context when the regex matches.

The only other related formula is portaudio but that will be handled in #196534

@samford samford added livecheck Issues or PRs related to livecheck CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. labels Nov 3, 2024
@github-actions github-actions bot added the long dependent tests Set a long timeout for dependent testing label Nov 3, 2024
@SMillerDev
Copy link
Member

Do we have a style check for this? Because .first seems like the Ruby way ™️ to me, so I can imagine that this will regress very quickly.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Don't really agree with this, sorry. But open to thoughts from @Homebrew/maintainers.

page.scan(regex).map { |match| match.first.tr("_", ".") }
page.scan(regex).map { |match| match[0].tr("_", ".") }
Copy link
Member

Choose a reason for hiding this comment

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

Strongly prefer match.first instead of match[0] TBH, and my impression is that we prefer the former more generally throughout the Homebrew codebase (barring Cask livecheck blocks)

@carlocab
Copy link
Member

carlocab commented Nov 3, 2024

Because .first seems like the Ruby way ™️ to me

Same, which is probably why we should stick to .first TBH. It's also more obvious, and is also I think more common in this repo outside of livecheck blocks (though could be wrong about that last one).

@samford
Copy link
Member Author

samford commented Nov 4, 2024

I agree that #first is more idiomatic Ruby and seems to be the preferred approach outside of livecheck strategy blocks (e.g., I use #first everywhere else).

The reason why numeric addressing is common in strategy blocks is because we also have a common pattern that uses #match but MatchData objects only allow numeric addressing. For example, when we use a regex with more than one capture group within a context where there will only be one match we use something like this (~300 instances in casks):

match = page.match(regex)
next if match.blank?

"#{match[1]},#{match[2]}"

Using numeric addressing when using #scan/#map is mostly intended to make the two approaches more similar, so maintainers/contributors don't have to remember that we use #first with #scan but have to use numeric addressing with #match.

However, we do have to remember that match[0] is the first capture when using #scan/#map but match[1] is the first capture when using #match. That's something that I've wanted to improve for years, so it would be nice to align these two areas for the sake of simplification.

I did some experimenting and came up with a slight tweak to the aforementioned #match pattern that would resolve the addressing mismatch and also allow us to use #first in both scenarios. Basically, if we use match = page.match(regex)&.captures instead, then match will effectively be the same array as when using #scan/#map.

All that said, if we prefer the #first approach, that's totally fine with me. I can rework the existing instances in homebrew/core and homebrew/cask accordingly if we decide to go in that direction.

@SMillerDev
Copy link
Member

I'm fine with either approach, but we need to have a style check if we want to force consistency.

@daeho-ro
Copy link
Member

daeho-ro commented Nov 4, 2024

If I use .first, then it is convenient only if there is only one match group. There are many examples with more match groups and the way to capture match groups are not unified, which makes me so confuse whether to select [0] or [1]. With this PR and the next step, it is possible to get rid of [0] and always use [1] for the first match group, then I will not be confuse. However, in other way, it can be used .first for the first match group and always start the next one with [2] with @samford 's way. So either is OK.

@MikeMcQuaid
Copy link
Member

I'm fine with either approach, but we need to have a style check if we want to force consistency.

I agree. There's no point in doing a mass cleanup like this without any sort of audit or style check to stop it backsliding. That should be done before this PR is merged, not after.

@samford samford marked this pull request as draft November 4, 2024 15:39
@samford
Copy link
Member Author

samford commented Nov 4, 2024

I'm fine with either approach, but we need to have a style check if we want to force consistency.

I'm currently reworking the livecheck block RuboCop setup to also apply to casks (i.e., it's only implemented using FormulaCop at the moment, so I'm converting it to a shared setup). I have it working for simple casks but now I'm accounting for blocks like on_ventura, on_arm, etc. (some of which also apply to formulae/resources). I'm planning to add more style checks like this once that's done and merged but it's naturally going to take some time.

The recent RuboCop PR got me thinking about this particular inconsistency, so I figured I would improve the status quo while it's top of mind. My thinking behind cleaning up the style in the interim time is that maintainers/contributors often copy from existing livecheck blocks when creating/updating others, so this could reduce the number of instances that would need to be cleaned up later.

I agree that we can't expect to maintain consistency if it isn't enforced with a style check but I also don't think that perfect needs to be the enemy of good in this case. However, if there isn't any wiggle room, then I can leave this as a draft or close it and open a new PR after creating a brew PR for new RuboCops.

@MikeMcQuaid
Copy link
Member

However, if there isn't any wiggle room, then I can leave this as a draft or close it and open a new PR after creating a brew PR for new RuboCops.

Draft seems good for now, thanks 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. livecheck Issues or PRs related to livecheck long dependent tests Set a long timeout for dependent testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants