feat: emit a warning when both package.publish and --index are specified
#16268
Conversation
rustbot
added
A-interacts-with-crates.io
motorailgun
force-pushed
the
publish-restriction-bypass
branch
from
d97bbc8 to
d97621a
Compare
|
Note that we ask for commits to be atomic (https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request). As an example of why this is useful, this PR extracts a method and adds a warning to it which takes more work for a reviewer to match things up to make sure nothing else changed unexpectedly and what the conditions for the warning are. Splitting this into two commits makes it more obvious. It is also great if the test is added in its own commit before the warning, with it passing (so no warning shown). Then the commit that adds the warning updates the test. The diff then makes it very clear what changed. |
|
Oops, that sounds very natural. Will fix it! |
motorailgun
force-pushed
the
publish-restriction-bypass
branch
from
d97621a to
8e32885
Compare
src/cargo/ops/registry/publish.rs
Outdated
| let opt_index_or_registry = opts.reg_or_index.clone(); | ||
| let registry_is_specified_by_any_package = pkgs | ||
| .iter() | ||
| .any(|pkg| pkg.publish().as_ref().map(|v| v.len()).unwrap_or(0) > 0); | ||
|
|
||
| let res = match (opt_index_or_registry, registry_is_specified_by_any_package) { |
There was a problem hiding this comment.
Why do we match on registry_is_specified_by_any_package? Feels like it would have been cleaner to isolate the conditional to the branch where it mattered. With the new setup, readers have to figure out what is intentionally different and why while with an interior if, it will be clearer.
There was a problem hiding this comment.
Oh, I somehow thought it was a good idea. The code should look better now, including fix for the reviewed part below
motorailgun
force-pushed
the
publish-restriction-bypass
branch
4 times, most recently
from
cb6f958 to
3684f52
Compare
motorailgun
force-pushed
the
publish-restriction-bypass
branch
from
3684f52 to
d4a80f3
Compare
|
Thanks! Makes your changes stand out a lot more |
rustbot
removed
the
S-waiting-on-review
3 participants
Partially adresses #16231.
What does this PR try to resolve?
cargo publishwill fail, if--registryis passed and that index isn't included inpackage.publishin Cargo.toml. However, as described in linked issue,--indexbypasses that check and may cause unexpected publication of packages.This PR implements warning that is shown when
--indexandpackage.publishis set at the same time.