Add a config option to suppress "license was not encountered" warnings #368
Conversation
thomcc
force-pushed
the
allow-allow-nonexhaustive
branch
from
5c53d14 to
1fe8a7c
Compare
|
(Uh, whoops, I guess after failing to locate the tests I... forgot to even try running |
|
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! |
|
Just out of curiosity, any thoughts on when a release will go out with this? No worries if you aren't sure.
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) |
|
There are several other changes that need a release, I will cut one sometime in the next week. |
|
Sorry I forgot about this, I'll cut a release later today. |
Thank you for the nice project! This is my first PR for this project. Therefore if I have any mistakes for contribution, please let me know... --- There are several reasons you might want this: 1. If the `allow` list represents the set sources 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 sources 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. ### Additional Information - This PR is for #781 - This PR modification is very similar with #368 - config name, `unused-allowed-source` is inspired by `licenses.unused-allowed-licens` in #368
2 participants
There are several reasons you might want this:
allowlist represents the set licenses for a project that have gone through some external approval process, such as vetting it with a legal department.deny.tomlfor all members, but not all members have identical dependency sets.deny.tomlas part of a project template, and configure it with some default set of licenses you find acceptableAs 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
denycould 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 aLintLevelseemed 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
.gitattributesin 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.