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 cargo deny init argument. #48

Merged
merged 5 commits into from
Dec 2, 2019
Merged

Add cargo deny init argument. #48

merged 5 commits into from
Dec 2, 2019

Conversation

foresterre
Copy link
Contributor

@foresterre foresterre commented Nov 29, 2019

Makes it possible to initialize a cargo deny configuration file.
The initialization command currently supports setting a few options directly:

  • unlicensed
  • copyleft
  • allow-osi-fsf-free
  • confidence-threshold

Of course, additional options can still be set by editing the cargo deny config
with an editor.

It is also possible to create a config file other than the default
/deny.toml (idea copied from the cargo deny check arguments) by providing --config <path-to-file>.

Checklist

  • I have read the Contributor Guide
  • I have read and agree to the Code of Conduct
  • I have added a description of my changes and why I'd like them included in the section below

Description of Changes

I worked on the init subcommand, which can be used to create a basic cargo-deny configuration file.

Related Issues

related: #42 (Add "init" subcommand)

Draft

Before this draft PR is finished:

I intend to at least add support for the allow and deny fields as options e.g.
cargo deny init --allow MIT Apache-2.0 ISC --deny AGPL-3.0-only AGPL-3.0-or-later

I also intend to add a test suite; however since none exists (for subcommands) this requires a few careful considerations and will take me a bit more time :).

Happy to make changes or add other additions if they are desired, in this PR or additional PRs 😃

@foresterre foresterre mentioned this pull request Nov 29, 2019
Copy link
Member

@Jake-Shadle Jake-Shadle left a comment

Choose a reason for hiding this comment

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

Hey, thanks for starting to implement this!

I think maybe this init subcommand is doing a bit too much right now though, I guess I should have been a bit more explicit in the original issue. But what I was thinking was something much simpler than this, just creating a toml from a the default config object, but then interspersing comments for each field/section similar to the info in the README.

The user is only going to do the init once for a project, but they are going to evolve the config over time in an editor, so the init is going to act more like a template than anything, and adding a bunch of options to a command they will run only a few times won't really "stick" as much as putting some "here are the possible values for this field" in the config file they will edit much more frequently, until they eventually get the hang of the config and probably delete the comments.

src/cargo-deny/init.rs Outdated Show resolved Hide resolved
Makes it possible to initialize a cargo deny configuration file.
The initialization command currently supports setting a few options directly:
* unlicensed
* copyleft
* allow-osi-fsf-free
* confidence-threshold

Of course, additional options can still be set by editing the cargo deny config
with an editor.

It is also possible to use a config file other than the default
deny.toml (idea copied from the cargo deny check arguments).
@foresterre
Copy link
Contributor Author

Thanks for the feedback! 🙂

I've removed the arguments (except for the config file path) and replaced it with a commented configuration file. The comments are based on (or copied where applicable) the comments from the readme. What do you think? 🙂

@Jake-Shadle
Copy link
Member

Nice! I guess I would just say to remove the comments on the actual example values so that they stand out more from the explanatory comments, then you don't need the additional guide comment at the top.

@foresterre
Copy link
Contributor Author

Nice! I guess I would just say to remove the comments on the actual example values so that they stand out more from the explanatory comments, then you don't need the additional guide comment at the top.

I am a bit confused which comments the "comments on the actual example values" are 🙄.
Do you mean the examples below [ban] (1) and the [[licenses.clarify]] example (2)? Or do you mean all comments (currently) starting with --?

(1) 57bb79d#diff-c012aa2026af44810754ffb835299323R120
(2) 57bb79d#diff-c012aa2026af44810754ffb835299323R75

@Jake-Shadle
Copy link
Member

Jake-Shadle commented Nov 29, 2019

Yes, I mean remove all the comments except the ones with -- then the user has a somewhat real file, that they will be able edit immediately to add remove things based on their needs, with the additional explanatory comments as inline help.

The specifics (especially important for the more exotic features
 can be found in the readme of the repo; this also keeps the
 maintenance of the template lower by having the detailed
 information available in a single place.

The current variant was inspired by the way a fresh .zshrc file
is formatted.
@foresterre
Copy link
Contributor Author

I cleaned up the template. Now each comment just gives an overview of what a config option means. Pre-filled values for lists have been removed. This was inspired by the template of a fresh .zshrc configuration file.

I see the following advantages:

  • easier to review the configuration by having less noise
  • having a single place where details need to be changed if they'll ever change (namely still the readme)

I still wasn't entirely sure what you mean with your previous comment; hopefully I understood correctly that you wanted the template config to contain less noise.
If you want something changed; I am happy to make the changes. Thanks for the fast feedback :).

In addition: I think it would be better to add the tests to a separate PR, which can also work towards fixing #1.

It was supposed to rename instead of see it as a different file...
Probably the contents where to different? Oops.
@Jake-Shadle
Copy link
Member

Yah, this is much more concise but still gives enough info that a new user can get a feel for what is available. And you have a link back to the README for more in depth info, so I'm pretty happy with this!

@foresterre foresterre marked this pull request as ready for review December 1, 2019 00:32
@Jake-Shadle
Copy link
Member

Ahh sorry, apparently github doesn't give a notification when a PR goes out of WIP. 😛

@Jake-Shadle Jake-Shadle self-requested a review December 2, 2019 17:16
@Jake-Shadle Jake-Shadle merged commit 2f349d4 into EmbarkStudios:master Dec 2, 2019
@foresterre foresterre deleted the feature/init branch December 2, 2019 20:47
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