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

Remove pattern matching usages #1

Closed

Conversation

Brcrwilliams
Copy link
Contributor

@Brcrwilliams Brcrwilliams commented Oct 17, 2022

The PackageURL gem currently uses pattern matching, which is an unstable experimental feature and is subject to change in future Ruby releases. On Ruby 2.7.5, a warning it outputted when the gem is require'd.

/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:87: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:115: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:110: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:140: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:152: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:166: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:181: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:252: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:272: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:299: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:293: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!
/Users/bwilliams/.asdf/installs/ruby/2.7.5/lib/ruby/gems/2.7.0/gems/packageurl-ruby-0.1.0/lib/package_url.rb:325: warning: Pattern matching is experimental, and the behavior may change in future versions of Ruby!

This discourages usage of the gem, as it could potentially break when upgrading Ruby versions.
This PR refactors the code so that it no longer uses pattern matching, in favor of more stable language features.

Pattern matching is not stable and may change with future versions of
Ruby. This refactors the PackageURL code and removes pattern matching
usage so that the gem won't potentially break when upgrading Ruby
versions.
@Brcrwilliams
Copy link
Contributor Author

@mattt Could you review this PR please? 🙏 I would love to use this gem in GitLab (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/100759#note_1132079448), but since pattern matching is experimental in Ruby I am worried that it won't be stable and could break when upgrading Ruby versions. I've done some refactoring to mitigate this.

@mattt
Copy link
Collaborator

mattt commented Oct 17, 2022

Hi @Brcrwilliams. Thanks for opening this PR.

Pattern matching is now stable as of Ruby 3 (see https://rubyreferences.github.io/rubychanges/3.0.html#pattern-matching). Do you know of a way to silence these warnings for anyone using this library with Ruby 2.7?

@Brcrwilliams Brcrwilliams changed the title Remove pattern matching Remove pattern matching usages Oct 18, 2022
end

def segment_present?(segment)
!segment.empty? && segment != '.' && segment != '..'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that the subpath and the namespace were parsed the same way. However, after studying https://github.com/package-url/purl-spec/blob/0b1559f76b79829e789c4f20e6d832c7314762c5/PURL-SPECIFICATION.rst#how-to-build-purl-string-from-its-components more carefully, it seems that the subpath should discard . and .. but the namespace can contain them. Is this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR to allow . and .. to be present in the namespace, to maintain the existing behavior.

@Brcrwilliams
Copy link
Contributor Author

@mattt These can be silenced with Warning[:experimental] = false, but there are some caveats:

  1. This has to be done before the first time the gem is required
  2. This will also hide other experimental warnings, so there is a risk of other experimental features being used in the codebase or in other libraries without any warning appearing for them. I would prefer not to silence the warnings because of this.

Overall, I feel this PR still provides a net benefit even with pattern matching being promoted to stable.

  1. The cognitive complexity of the parse and to_s methods has been reduced
  2. Additional test coverage has been added
  3. Code for parsing / encoding components of the PURL has been de-duplicated.

@Brcrwilliams
Copy link
Contributor Author

@mattt Is this PR something you might accept? If not, I'll need to figure out some sort of workaround for the warnings until we are able to upgrade to Ruby 3.

@Brcrwilliams
Copy link
Contributor Author

I have a different approach in #6 which may be better than this one.

@mattt
Copy link
Collaborator

mattt commented Nov 3, 2022

@Brcrwilliams Thank you for taking the time to look into this and for your patience in my following up on this.

The approach I took for this implementation was to minimize indirection between the implementation and specification. I started with the verbatim text of the PURL spec and did my best to translate it into Ruby code as literally as possible (kind of like Literate Programming). It's subjective, but I found Ruby's new pattern matching feature to provide the most direct mapping between the spec and executable code. The code may not be as DRY or modular as it could be if it were decomposed into smaller helper functions, but that's a trade-off I'm happy to make for an implementation that can be read linearly, stanza by stanza next to the spec.

As for the warnings generated by Ruby 2.7, I agree that the ergonomics aren't great and apologize for the inconvenience there. Because Ruby 2.7.6 will be EOL on March 31, 2023, my preference is to keep the existing implementation as-is rather than cater to a version of Ruby that won't be supported in a few months.

If you're deploying this library no Ruby 2.7 or earlier, I'd recommend either suppressing the warning or using your fork for the time being. I'll update the README to provide instructions for suppressing that warning in Ruby 2.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants