-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add pattern matching support to Gem::Version #9060
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
base: master
Are you sure you want to change the base?
Conversation
|
@baweaver Can you rebase this from the current master? I'm not sure why this PR didn't show Approved to run button for mainteners. |
colby-swandale
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor nit picks
488aafd to
906c9fc
Compare
Implements deconstruct and deconstruct_keys methods to enable
pattern matching on Gem::Version objects.
Array pattern matching:
case version
in [major, minor, patch]
# ...
end
Hash pattern matching:
case version
in major: 3.., minor: 2..
# ...
end
906c9fc to
423a743
Compare
| # | ||
| # Gem::Version.new("3.2").deconstruct_keys(nil) #=> { major: 3, minor: 2, patch: nil } | ||
| # Gem::Version.new("3").deconstruct_keys(nil) #=> { major: 3, minor: nil, patch: nil } | ||
| def deconstruct_keys(keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression RubyGems doesn't assume any particular versioning scheme beyond being dot-separated.
For example, "RubyGems Rational Versioning" in the class-level docs refers the third number as "build" rather than "patch". RubyGems also has released a version "3.2.0.rc.2".
I think only array patterns should be supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a fair argument, and I get where you're coming from here, but allow me a case for why it may still make sense.
1. Hash patterns provide greater readability
We can likely agree that major and minor are known. Patch vs build vs whatever other term? That's a fair point, but even with those two the intent is clearer with keywords:
# Hash pattern - clear intent
case version
in major: 3.., minor: 2..
use_modern_api
end
# Array pattern - requires mental mapping
case version
in [3.., 2.., _]
use_modern_api
end2. Partial matching without positional coupling
While versions are indeed well-structured it's easier to match without placeholders:
# Check major version without caring about minor/patch
case version
in major: 3..
# Works regardless of how many segments exist
end
# Array pattern requires placeholder for unused positions
case version
in [3.., _, _] # or [3.., *rest] - less clear
end3. Real world patterns
The most common use-cases tend to be around just checking major and minor version:
case Gem::Version.new(RUBYVERSION)
in major: 3.., minor: 2.. then gem "activesupport", "~> 8.0"
in major: 3.., minor: 1.. then gem "activesupport", "~> 7.0"
else gem "activesupport", "~> 6.0"
end4. Precedent in Ruby Core
Classes like Date and Time are also arguably positional in nature, and have more of an arbitrary precision element, but they also support hash-like patterns. I believe they are complementary for different use-cases, though again with time I'd not want to type out the array syntax to get what I wanted.
5. Edge cases
What should we do when the segments start to become a bit more erratic such as your case of 3.2.0.rc.2? Then it becomes fuzzier if it's a purely positional concept or if there are implied names for each given segment:
case version
in [major, minor, build, pre, _] when pre.is_a?(String)
# Handle prerelease
in major: 3.., minor: 2..
# Handle stable versions
endCompromise?
Given all of that I would still be inclined to support both, but update the documentation to:
- Note that hash keys (major/minor/patch) are conventional names, and not enforced semantics
- Show examples of pre-release / alpha / edge case versions
- Recommend array patterns for versions with 3+ segments
The hash pattern doesn't prevent flexibility, it provides a convenience mechanism for what's probably 90%+ of cases where people want to match against things, with array like patterns remaining available for more complex versioning schemes.
That said, it's your all's repo, so decision is yours there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(FWIW, I'm not a RubyGems maintainer and won't be deciding whether this gets merged.)
I'd expect array patterns and hash patterns can express roughly the same amount of information.
Personally, I don't agree with the 1. and 2., as array patterns are shorter and, to me, clear enough.
I'd probably use === matching with range ("3.2"..) when I don't have subpatterns to match against segments or need to extract them into variables, though.
| # case Gem::Version.new("3.2.1") | ||
| # in major: 3.., minor: 2.. | ||
| # # matches versions >= 3.2 | ||
| # end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would match "x.y" when x >= 3 && y >= 2, not when Gem::Version.new("x.y") >= Gem::Version.new("3.2").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, that is indeed a flaw of the hash-like patterns, so that comment would indeed be incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patched the docs to be more accurate there.
- Changed 'patch' to 'build' in documentation and return values - Added warning that hash patterns check segments independently - Added test demonstrating difference between hash patterns and version comparison - Fixed endless range syntax with parentheses
| # | ||
| # # This matches "3.2" but NOT "4.0" (since 0 < 2) | ||
| # case Gem::Version.new("4.0") | ||
| # in major: (3..), minor: (2..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the parens? Because if you exclude them the 2.. will ignore the newline and try and make 2.."matches" which is... not ideal. I'm half between whether that's a syntax bug or by design, but in the interim probably patch this to match.
| version = v("4.0") | ||
| result = | ||
| case version | ||
| in major: (3..), minor: (2..) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment on parens. TL;DR: without them it tries to do 2.."matches" because of the newline.
What was the end-user or developer problem that led to this PR?
Ruby 2.7+ introduced pattern matching, but
Gem::Versiondoes not support it. This makes version-based conditional logic more verbose than necessary, particularly when selecting dependencies based on Ruby version.The current approach requires multiple conditionals:
What is your fix for the problem, implemented in this PR?
Added
deconstructanddeconstruct_keysmethods toGem::Versionto enable pattern matching syntax.Implementation:
deconstructaliases the existing segments method for array pattern matchingdeconstruct_keysextracts major, minor, and patch components from segmentsThis enables version-based logic:
Make sure the following tasks are checked
• [x] Describe the problem / feature
• [x] Write tests for features and bug fixes
• [x] Write code to solve the problem
• [x] Make sure you follow the current code style and write meaningful commit messages without tags