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 config option to suppress "license was not encountered" warnings #368

Merged
merged 2 commits into from
Oct 1, 2021

Conversation

thomcc
Copy link
Contributor

@thomcc thomcc commented Sep 30, 2021

There are several reasons you might want this:

  1. If the allow list represents the set licenses for a project that have gone through some external approval process, such as vetting it with a legal department.
  2. You're checking a single project in a workspace that shares its deny.toml for all members, but not all members have identical dependency sets.
  3. You'd like to use deny.toml as part of a project template, and configure it with some default set of licenses you find acceptable
  4. ... others, for sure...

As it is, this warning isn't a huge deal, but is annoying/unhelpful if you don't care about it. I suspect that it's useful for catching typos or keeping configuration tight (and so I think "warn" is the right default for it), but there are enough reasons to want to turn it off that it seems justified to me for it to be an option.

It was easy to add support for a config property which controls the lint level for this check, so I just did that. I guess setting it to deny could be desirable in some cases, although it seems a little dodgy to me for various reasons... That said, I didn't see a reason to forbid that sort of thing, and allowing it to be configured as a LintLevel seemed more consistent.

Regarding the code:

I don't care about the name of this property, so if you'd like me to change it LMK. I think this name is clear, but it is a bit verbose and maybe there's something better or more consistent. (Unfortunately, a lot of the more obvious names I thought of were actually confusing because of the usage of "allow" to refer to both the lint level and the list being linted)

I've tested it manually, since AFAICT this code isn't covered by tests. Or maybe I just missed them.


P.S. I also fixed the .gitattributes in the first commit. I'm fine dropping that commit (it's unrelated to the patch and not really my business) but that file is binary, and so it shouldn't be marked as text. Without this, git kept insisting on replacing CRLF with LF in it (or maybe it was the other way around), which was very annoying, and definitely does not result in a valid zstd file.

@thomcc
Copy link
Contributor Author

thomcc commented Sep 30, 2021

(Uh, whoops, I guess after failing to locate the tests I... forgot to even try running cargo test. My bad 😓)

@Jake-Shadle
Copy link
Member

Oh, weird about the zstd file, I take it you are on Windows?

Anyway, I am ok with this change, I've definitely hit this before myself just never bothered to fix it, so thanks for the PR!

@mergify mergify bot merged commit 128ae2c into EmbarkStudios:main Oct 1, 2021
@thomcc
Copy link
Contributor Author

thomcc commented Oct 1, 2021

Just out of curiosity, any thoughts on when a release will go out with this? No worries if you aren't sure.

Oh, weird about the zstd file, I take it you are on Windows?

I'm on macOS 🤷. That said, it's a new machine, so I don't have any config set for handling CLRF in a special way or whatever like autoclrf/safecrlf. (It's also one of the ARM ones which probably doesn't make a difference but who the hell knows)

@Jake-Shadle
Copy link
Member

There are several other changes that need a release, I will cut one sometime in the next week.

@Jake-Shadle
Copy link
Member

Sorry I forgot about this, I'll cut a release later today.

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.

None yet

2 participants