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

Running cargo semver-checks with no other arguments should default to checking #459

Merged
merged 9 commits into from
May 31, 2023

Conversation

era
Copy link
Contributor

@era era commented May 30, 2023

Addresses #414.

It allows users to run cargo semver-checks without using the subcommand check-release.

I basically moved CheckRelease to be a field at SemverChecks.

I tried to also remove it from the SemverChecksCommands::CheckRelease enum by using global = true on all fields from CheckRelease, but I could not add that property to manifest and workspace fields.

I reused the tests from feature_config to make sure the same use cases work when user specifies the subcommand or not.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Love the tests, great idea to use the existing test setup to ensure this functionality works too!

Just a few minor suggestions for further improvement.

src/main.rs Outdated
Comment on lines 90 to 95
let report = check.check_release()?;
if report.success() {
std::process::exit(0)
} else {
std::process::exit(1);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could we refactor the code here a bit to avoid the duplicated logic?

Maybe something like:

let check: cargo_semver_checks::Check = args.command.unwrap_or_else(|| args.check_release.into());

let report = check.check_release()?;
// ...

src/main.rs Show resolved Hide resolved
tests/feature_config.rs Outdated Show resolved Hide resolved
Comment on lines 43 to 49
CargoSemverChecks::new_with_check_release_subcommand(
"test_crates/features_simple/new/",
"test_crates/features_simple/old/Cargo.toml",
)
.add_arg("--only-explicit-features")
.run()
.success();
Copy link
Owner

Choose a reason for hiding this comment

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

This duplication is a bit unfortunate: over time as we edit these tests and add new ones, we might update one version of the check but fail to update the other. This is a maintainability risk.

Could you think of another way to set up the test where we don't need to duplicate all the test code like this? Perhaps by doing some more refactoring of the CargoSemverChecks type?

@era
Copy link
Contributor Author

era commented May 31, 2023

Let me know if the tests are better now, I thought of adding an assert(fn(Assert) -> ()) -> () function to CargoSemverChecks, but I was not sure if exposing the vector wouldn't be the best move.

Also, not sure about the way I remove the subcommand from args. Let me know if you think it could be better.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Nicely done! A couple of tiny tweaks and this is good to go 🚀

src/main.rs Outdated Show resolved Hide resolved
tests/feature_config.rs Outdated Show resolved Hide resolved
tests/feature_config.rs Outdated Show resolved Hide resolved
@era
Copy link
Contributor Author

era commented May 31, 2023

Thank you again @obi1kenobi for all the reviews!

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Awesome, let's ship it 🚀

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