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

Extend gemspec pattern matching #246

Merged
merged 4 commits into from
Dec 13, 2017
Merged

Extend gemspec pattern matching #246

merged 4 commits into from
Dec 13, 2017

Conversation

jonabc
Copy link
Contributor

@jonabc jonabc commented Dec 13, 2017

Found a few issues with parsing the license field out of a gemspec.

  1. The gem specification allows for a licenses array field that wasn't found and captured.
  2. The pattern didn't match on frozen strings, e.g. "mit".freeze

Ruby allows regex's to be interpolated inside other regex's which makes the array regex much easier to understand and work with. It also made it simpler to ensure values were treated equally among all of the regular expressions.

The only captured group in these regex's is the license text. All other groups are non-capturing with (?: ...). Meaning in an array like ["mit", "bsd-3-clause"], match[1] == "mit" and match[2] == "bsd-3-clause"

Only the first array value is used, to match current functionality.

@mlinksva
Copy link
Contributor

If there are multiple values in a licenses array I suspect the best match for current functionality would be to return other. That's what licensee does when it encounters multiple licenses, whether from different (eg README and license file) or the same matchers, eg when it finds a license expression in npm packages (though it doesn't attempt to parse out the multiple licenses).


# this will capture each value in the license array
# for now only return the first match
match = @file.content.match LICENSE_ARRAY_REGEX
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, we'd abstract things such that a project file could have multiple licenses, but for now, I agree with @mlinksva that "other" would be the consistent response here (but I'd leave the other logic intact to allow for that). Perhaps expose a matches method for now (in addition to match)?

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 avoided adding any new fields at a broad scope because I wasn't sure how they would be used. It definitely seems worth thinking about for the future.

@jonabc
Copy link
Contributor Author

jonabc commented Dec 13, 2017

@benbalter @mlinksva thanks for taking a look! I've updated per feedback, though I'm not sure why the dependency-ci job failed 🙁

@benbalter
Copy link
Contributor

I'm not sure why the dependency-ci job failed

Probably just added that to test the service. No idea. Safe to ignore.

Copy link
Contributor

@benbalter benbalter left a comment

Choose a reason for hiding this comment

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

👾

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

Successfully merging this pull request may close these issues.

None yet

3 participants