-
-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
use configured required update version #800
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation portion looks good. I have a couple of questions and concerns about some of the test setup. Nothing major, should be easy to sort out.
resolver = "2" | ||
members = ["pkg"] | ||
|
||
[workspace.metadata.cargo-semver-checks] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might we want to add another layer of indirection here, like workspace.metadata.cargo-semver-checks.lints
? With the current design, our entire metadata entry is reserved for lints and we can't add any other kind of config data at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I can open a separate PR for this.
tests/workspace_lint_config.rs
Outdated
"--baseline-root", | ||
"baseline", | ||
"--manifest-path", | ||
"current/pkg/Cargo.toml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you either make sure to either make a new subdirectory for all test cases for package/workspace config testing, or follow the existing norms in the directory where you're adding the test crates?
Using baseline/current
instead of old/new
will probably break some of the tooling we use, such as this script for example:
cargo-semver-checks/scripts/regenerate_test_rustdocs.sh
Lines 46 to 48 in f8f89ec
if [[ -f "$TOPLEVEL/test_crates/$crate_pair/new/Cargo.toml" ]]; then | |
if [[ -f "$TOPLEVEL/test_crates/$crate_pair/old/Cargo.toml" ]]; then | |
for crate_version in "new" "old"; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use the regenerate_test_rustdocs.sh
because we are passing the manifest path to cargo-semver-checks
, so we don't want to generate rustdoc for these crates, which is why I didn't use new and old. It does make more sense to move them into a subdirectory, though.
tests/workspace_lint_config.rs
Outdated
|
||
// 1 minor: function_missing, overridden in workspace and not overridden in package | ||
// 1 major: module_missing, overridden in workspace (minor) then overridden in package (major) | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this empty comment line intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
members = ["pkg"] | ||
|
||
# [workspace.metadata.cargo-semver-checks] should only be read from the | ||
# current Cargo.toml (this is the baseline) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a great test case to add in a future PR!
// pub fn function_missing() {} | ||
// pub mod module_missing {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm generally not a fan of commented-out code by itself, without any explanation or context.
Consider adding a brief doc comment explanation to the lib.rs
in both current
and baseline
that describes the test case: what it tries to catch and how, and mention why this code is commented out here.
That will be super useful in the future, if this test case starts failing or needs to be modified by a future PR. We certainly won't remember why this code is commented out, and it's much easier to take 30s to write down that context now than to spend half an hour reconstructing it from scratch later.
8216d8d
to
f34189f
Compare
* add comments to workspace test files * remove extraneous line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could tweak the workspace_lint_config.rs
check to be a bit more stringent, I feel good about the rest of this change and would be happy to merge it so we can move forward.
"--manifest-path", | ||
"new/pkg/Cargo.toml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible other test: pass --manifest-path new/Cargo.toml
-- the workspace manifest instead of the individual crate manifest -- and assert that the behavior is the same.
We want to make sure that crates are scanned with their crate's settings, even when the scan is triggered at workspace level.
This is fine to add in a subsequent PR if that's easier.
run_check_release
functioneffective_required_update
function in place ofquery.required_update
whenever we previously accessed it to use the configured valuenote: in
lib.rs:460
, when we don't have access to the current manifest, we can't read config at all, so we pass in an empty stack. If we were to read CLI config, we could store that inGlobalConfig
and put that at the bottom of the stack for both situations we pass overrides