Skip to content

version: extend token comparisons to strings, integers, and nil#8186

Merged
SeekingMeaning merged 3 commits intoHomebrew:masterfrom
SeekingMeaning:version/token-comparisons
Aug 3, 2020
Merged

version: extend token comparisons to strings, integers, and nil#8186
SeekingMeaning merged 3 commits intoHomebrew:masterfrom
SeekingMeaning:version/token-comparisons

Conversation

@SeekingMeaning
Copy link
Copy Markdown
Contributor

@SeekingMeaning SeekingMeaning commented Aug 2, 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?

This is useful for comparing version tokens, especially with #8183, for example:

Formula["gcc"].version.major > 6

@SeekingMeaning SeekingMeaning added the in progress Maintainers are working on this label Aug 2, 2020
Comment thread Library/Homebrew/version.rb Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
version.scan(SCAN_PATTERN).map! { |token| Token.create(token) }
version.scan(SCAN_PATTERN).map { |token| Token.create(token) }

@SeekingMeaning SeekingMeaning force-pushed the version/token-comparisons branch from 9a95c31 to ab18982 Compare August 2, 2020 19:32
@SeekingMeaning SeekingMeaning removed the in progress Maintainers are working on this label Aug 2, 2020
@SeekingMeaning SeekingMeaning marked this pull request as ready for review August 2, 2020 19:59
end

def <=>(other)
return unless other = Token.from(other)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be returning nil or -1? It feels like this should maybe be part of the else case?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nil represents that the two types are not comparable. In this case, if other isn't a valid token (and cannot be converted to one).

This line will also do a conversion from int/string to a token before hitting the case checks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works for me, thanks @Bo98!

@SeekingMeaning SeekingMeaning merged commit 0653bb1 into Homebrew:master Aug 3, 2020
@SeekingMeaning SeekingMeaning deleted the version/token-comparisons branch August 3, 2020 17:10
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Dec 21, 2020
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 21, 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.

5 participants