Modify StringToken::PATTERN to fix comparison #8125
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
brew style
with your changes locally?brew tests
with your changes locally?StringToken
'sPATTERN
regex (/[a-z]+[0-9]*/i
) is consuming numeric parts of the version string that should probably be tokenized separately as aNumericToken
. As a result, you can end up with incorrect version comparisons when part of the version string consists of letters with trailing numbers.Taking Homebrew/homebrew-core#58711 as an example,
4.0ga10
is treated as older than4.0ga9
. This is because4.0ga10
is currently tokenized as follows:When
ga10
is compared toga9
(i.e.,"ga10" <=> "ga9"
), it's reported as being lower (-1
). To address this, we can comparega
and10
/9
separately (as aStringToken
andNumericToken
respectively).This PR removes the
[0-9]*
fromStringToken
'sPATTERN
regex, which results in the aforementioned version being tokenized as follows:This produces the comparison result that we would expect, where
4.0ga10
is treated as newer than4.0ga9
.I did a full
livecheck
run over the homebrew-core formulae with/without the change in this PR and the only difference was withx3270
. At least from the perspective oflivecheck
, this change doesn't seem to result in any undesirable version comparison issues.I've left this as a draft for the moment because I have a question about how we should be doing
StringToken
comparisons. It seems like we should treatga
andGA
as the same, otherwise4.0ga10
would be treated as newer than4.0GA11
. To address this, we would seemingly need to modifyStringToken
comparisons to ignore case.Is anyone aware of any situations where letter case is used to indicate something newer (e.g., cycling through lowercase a-z before proceeding to uppercase A-Z)? Once we've decided how to proceed in this area, I'll add some additional tests and mark this as ready for review.