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

Add a --clarificationFile option #64

Merged
merged 7 commits into from
Feb 18, 2023
Merged

Add a --clarificationFile option #64

merged 7 commits into from
Feb 18, 2023

Conversation

mikayla-maki
Copy link
Contributor

While building out my company's automated licensing compliance code I attempted to use this project to implement automated monitoring for our project's compliance with the --onlyAllow flag. However, several of our dependencies (e.g. @contentlayer/cli@0.3.0) have an UNKNOWN license. I could not find a way to add package specific overrides with this project, so I decided to contribute to this project :).

This PR adds a --clarificationFile option, modeled after the similar behavior in cargo-about. This allows users to specify custom overrides for specific packages with missing or otherwise malformed fields. I also added tests for the feature, a clarificationExample.json file to show how it should work, and ensured that code coverage stayed within the project's limits.

Let me know if there's anything else I can do for this PR, and thank you for maintaining such a vital project!

This commit adds a --clarificationFile option, modeled after the behavior of `cargo-about`. It allows users to specify clarifications for specific packages and versions with missing or otherwise malformed fields.
@mikayla-maki
Copy link
Contributor Author

mikayla-maki commented Feb 2, 2023

On further usage, I think it would be better to make the checksum field optional. I'm used to Cargo's packaging norms and NPMs aren't as standardized unfortunately :(

@RSeidelsohn
Copy link
Owner

Wow, this is by far the best PR I've seen so far for this project since I forked it from the original license-checker - thank you so much!
I will have a look most likely tomorrow, but please bear with me if I can't do it before next week. I'm in winter holidays currently, using my free time for this project while the rest of my family is out for skying. Tomorrow will be the last day and it might be that I won't find even the little time necessary for having a look, even though I have the feeling that I can blindly trust your PR.
So getting back home will take us two days, but next week I will still be free. Should it be really urgent, please tell me so and I will put more pressure on my timetable.

Getting back to this PR soon & cheers,
Roman.

@mikayla-maki
Copy link
Contributor Author

Hey Roman,

Enjoy your holiday with your family, no rush to merge this PR! I have already pointed our internal builds at my fork branch so we're humming along just fine :)

Hope the skiing shreds 😄

@mikayla-maki
Copy link
Contributor Author

In attempting to use this I found a need to snip out portions of the license from other embedded files. I have added the relevant functionality as well as some more documentation for the new features :)

Copy link
Owner

@RSeidelsohn RSeidelsohn left a comment

Choose a reason for hiding this comment

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

Hi @mikayla-maki, once more thank you so much for your great contribution!
I have 3 really minor change requests of a purely aesthetic nature, I hope you don't mind.

I currently still struggle with setting up a proper Prettierjs and ESlint configuration that works seamlessly with the Sublime Text 4 setup I am using here, which will - should it work - demand single quotes rather than double quotes where applicable, but I hope this will not be an issue here for now.
What also irritates me is the fact that PRs come from foreign repositories, but that's a thing I have to learn, as it's the usual way things go in open source world. So maybe I handle things a bit strangely from time to time, but I hope my workflow and the project's documentation will improve over time.
That being said: I'm always open for questions and suggestions.

Cheers, Roman.

tests/clarificationFile-test.js Outdated Show resolved Hide resolved
bin/license-checker-rseidelsohn Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
mikayla-maki and others added 2 commits February 13, 2023 12:28
Co-authored-by: Roman Seidelsohn <rseidelsohn@gmail.com>
@mikayla-maki
Copy link
Contributor Author

@RSeidelsohn Aesthetic changes implemented! Thank you for the feedback :D

@RSeidelsohn RSeidelsohn merged commit d95e43e into RSeidelsohn:master Feb 18, 2023
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.

2 participants