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

Only consider license content until 'END OF TERMS AND CONDITIONS' #154

Merged
merged 2 commits into from Feb 21, 2017

Conversation

3 participants
@talisein
Contributor

talisein commented Dec 28, 2016

The GPL licenses have 'How to apply to your program' instructions at the bottom of their file. This text is not the license itself, and some people remove the instructions when putting the LICENSE file in their repo.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Dec 30, 2016

Owner

Thanks @talisein!

@mlinksva any strong feelings one way or the other?

Owner

benbalter commented Dec 30, 2016

Thanks @talisein!

@mlinksva any strong feelings one way or the other?

@mlinksva

This comment has been minimized.

Show comment
Hide comment
@mlinksva

mlinksva Dec 30, 2016

Collaborator

Seems fine to me so long as it doesn't lead us to stop matching with instructions included. Are there tests for matching both normal and instructions-removed versions of the texts this truncates for matching purposes?

Collaborator

mlinksva commented Dec 30, 2016

Seems fine to me so long as it doesn't lead us to stop matching with instructions included. Are there tests for matching both normal and instructions-removed versions of the texts this truncates for matching purposes?

@mlinksva

This comment has been minimized.

Show comment
Hide comment
@mlinksva

mlinksva Dec 30, 2016

Collaborator

(Even if they match now, tests are important in case we separately decide to further tweak matching thresholds, etc.)

Collaborator

mlinksva commented Dec 30, 2016

(Even if they match now, tests are important in case we separately decide to further tweak matching thresholds, etc.)

@talisein

This comment has been minimized.

Show comment
Hide comment
@talisein

talisein Dec 30, 2016

Contributor

I added a test for matching when the repository under test is GPLv3 with no instructions. There are existing tests where the repository under test includes the instructions (e.g. fixtures/lgpl). If you would like me to create explicit tests for all the licenses this effects today (Apache, AGPL, LGPL, GPL) I don't mind, but it might clutter up the fixtures directory a little.

(I should add that my first stab at this patch, where I edited the license files fetched from chooosealicense, broke the existing lgpl test because it would not match to a license anymore)

Contributor

talisein commented Dec 30, 2016

I added a test for matching when the repository under test is GPLv3 with no instructions. There are existing tests where the repository under test includes the instructions (e.g. fixtures/lgpl). If you would like me to create explicit tests for all the licenses this effects today (Apache, AGPL, LGPL, GPL) I don't mind, but it might clutter up the fixtures directory a little.

(I should add that my first stab at this patch, where I edited the license files fetched from chooosealicense, broke the existing lgpl test because it would not match to a license anymore)

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Jan 3, 2017

Owner

The technical implementation is fine here (thanks @talisein). Between this and the Apache boilerplate, I'm wondering if licenses (at least those two), should have an alternate form, which we could test. I'd prefer we test the versions that the vast majority use (which could result in an exact match, with 100% confidence) than a mangled version that's still above our 95% threshold (which we may one day want to raise or lower, which could result in false positive/negatives).

Owner

benbalter commented Jan 3, 2017

The technical implementation is fine here (thanks @talisein). Between this and the Apache boilerplate, I'm wondering if licenses (at least those two), should have an alternate form, which we could test. I'd prefer we test the versions that the vast majority use (which could result in an exact match, with 100% confidence) than a mangled version that's still above our 95% threshold (which we may one day want to raise or lower, which could result in false positive/negatives).

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Feb 21, 2017

Owner

I'd prefer we test the versions that the vast majority use (which could result in an exact match, with 100% confidence) than a mangled version that's still above our 95% threshold

@talisein Looking at your implementation again, I changed my mind and like your approach. If a license says "END OF TERMS AND CONDITIONS", it should be safe to ignore anything after it from an enforceability perspective, and as long as we're doing it on both sides, as you are, the result should be the same.

I'm going to resolve the merge conflict and get this merged.

Owner

benbalter commented Feb 21, 2017

I'd prefer we test the versions that the vast majority use (which could result in an exact match, with 100% confidence) than a mangled version that's still above our 95% threshold

@talisein Looking at your implementation again, I changed my mind and like your approach. If a license says "END OF TERMS AND CONDITIONS", it should be safe to ignore anything after it from an enforceability perspective, and as long as we're doing it on both sides, as you are, the result should be the same.

I'm going to resolve the merge conflict and get this merged.

@benbalter benbalter merged commit e8762b0 into benbalter:master Feb 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 99.316%
Details
@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Feb 21, 2017

Owner

@talisein Thanks for this. Resolved the merge conflict and merged via #154.

Owner

benbalter commented Feb 21, 2017

@talisein Thanks for this. Resolved the merge conflict and merged via #154.

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