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

implement basic check flag #17

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

implement basic check flag #17

wants to merge 3 commits into from

Conversation

azdle
Copy link

@azdle azdle commented Apr 9, 2024

This implements a very basic version of a check flag. If any dependencies that should move to the workspace are found, they are printed to stderr and the command exits with a FAILURE ExitCode.

If any dependencies are found in a workspace member, but that have an existing workspace dependency that can be directly used (that is if the workspace Cargo.toml doesn't need to be modified) nothing is printed, but the command still exits with FAILURE.

Ideally the check flag would work like cargo fmt --check where a diff is shown, but that would require extensive refactoring.


Not sure if you'll be okay with ^ limitations. If not I may be able to find some time in the future to try making proper diffs possible, not sure.

Also, I've added ExitCode as the return from auto_inherit in lib.rs. That's more of a command specific thing, but I thought it would be good to differentiate between "failed to run the command" and "the command ran successfully, but indicated failure". I can make it something else if you find using std::process::ExitCode for that too gross.

resolves #2

@sgasse sgasse mentioned this pull request Apr 30, 2024
@hdoordt hdoordt self-requested a review October 29, 2024 12:45
Copy link
Member

@hdoordt hdoordt left a comment

Choose a reason for hiding this comment

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

Hi @azdle, sorry about the delay!

I had a look at your changes, and it looks good in general. I think we can live without the diff for now. About this part in your description, though:

If any dependencies are found in a workspace member, but that have an existing workspace dependency that can be directly used (that is if the workspace Cargo.toml doesn't need to be modified) nothing is printed, but the command still exits with FAILURE.

Would you mind updating the code such that an informative error gets printed in these cases?

@hdoordt
Copy link
Member

hdoordt commented Oct 29, 2024

I rebased on main just now

@hdoordt hdoordt force-pushed the check branch 2 times, most recently from ee983fc to 344a695 Compare October 30, 2024 07:59
@azdle
Copy link
Author

azdle commented Oct 30, 2024

If any dependencies are found in a workspace member, but that have an existing workspace dependency that can be directly used (that is if the workspace Cargo.toml doesn't need to be modified) nothing is printed, but the command still exits with FAILURE.

Would you mind updating the code such that an informative error gets printed in these cases?

That's part of what would require pretty extensive refactoring. The thing I could do is have separate error messages for "crate shouldn't have this" and "workspace should have this" if you're okay with split/duplicated error messages.

I've pushed up a commit with that option.

I also pushed up a couple test cases to show what I mean:

$ cd new-child-dep/
$ cargo run --manifest-path ~/downloads/cargo-autoinherit/Cargo.toml -- autoinherit --check
   Compiling cargo-autoinherit v0.1.5 (~/downloads/cargo-autoinherit)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.87s
     Running `~/downloads/cargo-autoinherit/target/debug/cargo-autoinherit autoinherit --check`
`regex` should be declared as a workspace dependency.
`regex` should inherit from a workspace dependency.
$ cd ../uninherited-existing-child-dep/
$ cargo run --manifest-path ~/downloads/cargo-autoinherit/Cargo.toml -- autoinherit --check
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
     Running `~/downloads/cargo-autoinherit/target/debug/cargo-autoinherit autoinherit --check`
`regex` should inherit from a workspace dependency.

AFAIK, there's currently no one place where it's possible to decide if something needs to be changed in both the workspace toml and the crate toml.


I think a better way to do all of this would be to separate planning what needs to be changed for all files from actually modifying all the files. So that at some point you have a list of all the changes that need to be made and you can decide if you just print them out or actually make the change. (I think this would also make it a lot easier to implement #16. It should also make it easier to make proper diffs or at least have crate names in the error messages.)

I've got a bit of time on my hands at the moment, would you be interested in a refactor PR that does that instead of this PR?

@hdoordt
Copy link
Member

hdoordt commented Nov 1, 2024

@azdle

would you be interested in a refactor PR that does that instead of this PR?

I think it's a good idea to separate the planning from the actual modification. I think the best way to go about this is by setting up some sort of small proof-of-concept that we can then iterate on.

@LukeMathWalker What do you think?

azdle and others added 3 commits November 1, 2024 14:59
This implements a very basic version of a check flag. If any
dependencies that should move to the workspace are found, they are
printed to stderr and the command exits with a FAILURE `ExitCode`.

If any dependencies are found in a workspace member, but that have an
existing workspace dependency that can be directly used (that is if the
workspace Cargo.toml doesn't need to be modified) nothing is printed,
but the command still exits with FAILURE.

Ideally the check flag would work like `cargo fmt --check` where a diff
is shown, but that would require extensive refactoring.
@hdoordt
Copy link
Member

hdoordt commented Nov 1, 2024

Rebased on main again

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.

Add a subcommand to verify all dependencies are inherited
2 participants