ryanuber / go-license

Software licensing standardization library for Golang
MIT License
69 stars 15 forks source link

#8 -- Case-Insensitive File Matching #9

Closed client9 closed 9 years ago

client9 commented 9 years ago

for your review... the existing tests actually seem to do exercise some of this already. Let me know if you want more tests.

ahmetb commented 9 years ago

mind adding a test case?

client9 commented 9 years ago

yup.. I 'll sort it out with a test.

client9 commented 9 years ago

ok take a look.. I changed the existing test to use mixed case for license file (implicit check), and then added tests for the case insensitive matching explicitly.

ryanuber commented 9 years ago

Added a few comments but looking good! Thanks for tackling this.

client9 commented 9 years ago

updated with exception of "Now that I'm thinking about this more, we should probably report if there is more than a single license file found that we recognize, to avoid reporting the wrong license type." since that might require an API change, and is a separate bug.

ryanuber commented 9 years ago

@client9 does erroring on multiple matched licenses force an API change? Both NewFromDir() and GuessFile() return an error, which should be sufficient to report a static error to the tune of "Multiple possible licenses found, cannot guess type". I also understand that this problem exists in the current code, and if you'd rather someone else tackle that in a different PR, that's ok, but we are touching all of the relevant code here so I thought we might as well get it all right. Just let me know!

client9 commented 9 years ago

Ahh, ok. I was think you meant changing

type License struct {
    Type string // The type of license in use
    Text string // License text data
    File string // The path to the source file, if any
}

to be File []string or something.

Returning an error if multiple matches is simple, and makes sense.

will do later today.

thx

n

client9 commented 9 years ago

update to err if there are multiple licenses found in a directory.

ryanuber commented 9 years ago

Looks good! I think at this point we are just missing a test for multiple license errors.

client9 commented 9 years ago

Ah yes. ok good will do shortly

client9 commented 9 years ago

re: https://github.com/tools/godep/issues/245#issuecomment-148947785

client9 commented 9 years ago

sample license that would have been missed by old implementation:

https://github.com/facebookgo/dvara/blob/master/license

ryanuber commented 9 years ago

LGTM. Thanks!