iandotkelly / nlf

Node License Finder
MIT License
153 stars 41 forks source link

Deal with misnamed 'licenses' keys in package.json #22

Closed myndzi closed 9 years ago

myndzi commented 9 years ago

Deals with https://github.com/iandotkelly/nlf/issues/21

iandotkelly commented 9 years ago

Hey, I'm sorry ..... I saw the report and fixed it myself before I saw this PR.

Thanks very much for the PR !

myndzi commented 9 years ago

Haha no worries. I sent the pull request when I realized the problem was different. This patch also addresses an object being supplied as the 'licenses' value; I prefer this solution since it literally treats a single item the same as the array contents.

iandotkelly commented 9 years ago

@myndzi .... It does address that, but that would be a quite surprising thing to do - what would the object have as properties and values? The case where they mix up the license and licenses properties (such as cookie-parser does) is somewhat more understandable.

It might be just better if someone had "unknown" as the license for a module in the case that it is not a string or an array - forcing you to make the effort to establish the license manually.

myndzi commented 9 years ago

I actually encountered exactly one such case :) https://github.com/mozilla/node-convict/blob/master/package.json

iandotkelly commented 9 years ago

@myndzi - aha!

It seems that the licenses property has even been officially dropped from package.json - but that some people are campaigning for it to be returned, see https://github.com/npm/npm/issues/4473

If you look the first post in that issue is someone suggesting the same {type: "", url: ""} object that you found in node-convict.

I'll have a think about how I want to address this - if URLs are provided should I include those for example.

myndzi commented 9 years ago

Did you ever decide to deal with this? I have to use my fork to correctly detect convict's license or add some kind of exception rule, which is undesirable.

iandotkelly commented 9 years ago

@myndzi - ok, sorry, lost track of this conversation.

If you change the spacing from spaces to tabs in the PR i'll merge this. Just to be consistent in the project and file.

Ideally you would add a test for this too.

iandotkelly commented 9 years ago

Thanks for the PR and for pointing out this issue.

There was a conflict in the merge, so when resolving this I decided that the fix was in the wrong place and was duplicating some of the functionality of the addPackageJson function, which was essentially the place responsible for adding licenses from package.json.

I've put my own fix in place, with a test (https://github.com/iandotkelly/nlf/commit/862342bd311eeebd7539002c792ccfe1df93d5b4) and have released on npm@1.3.2

I've tested this on the node-convict module, and it seems to work - let me know if it doesn't meet your needs. I'll release updates to gulp-license-finder and grunt-license-finder as well.

myndzi commented 9 years ago

I was surprised when you asked for the PR to be updated since you'd already modified code with the same intent :) Thanks for the fix.