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.

Sign up for GitHub

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

Jump to bottom

Add better error message when it can not find the search section #12865

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Add better error message when it can not find the search section #12865

merged 1 commit into from
Nov 7, 2023

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Oct 22, 2023 •
edited
Loading

What does this PR try to resolve?

close #12405

Try to give a better error message when it can not find the specific section.

How should we test and review this PR?

I updated all broken tests. You can check out these tests.

Additional information

None

@rustbot
Copy link
Collaborator

rustbot commented Oct 22, 2023

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-remove S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 22, 2023
@Rustin170506
Copy link
Member Author

Rustin170506 commented Oct 22, 2023 •
edited
Loading

Hi @epage, do you have any suggestions here? It seems we use the API from the local manifest to modify the Cargo.toml.

cargo/src/cargo/ops/cargo_remove.rs

Line 49 in a2a2cf0

manifest.remove_from_table(&dep_table, dep)?;

I'm not entirely certain whether it would make sense to change the API to include fallback_search_tables or if there's a better API design available.

@epage
Copy link
Contributor

epage commented Oct 23, 2023

I'm not entirely certain whether it would make sense to change the API to include fallback_search_tables or if there's a better API design available.

remove_from_table is already dependency-specific. I would pass non_existent_dependency_err and the raw table_path to non_existent_dependency_err and have it walk all dependency tables (hard coded), skipping over table_path, checking if its present and include it in building up in the error.

The caller of the op shouldn't be responsible for generating fallback_search_tables.

@rustbot rustbot added the A-manifest Area: Cargo.toml issues label Oct 26, 2023
@rustbot rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Oct 27, 2023
@Rustin170506 Rustin170506 force-pushed the rustin-patch-remove branch 4 times, most recently from 42a1a02 to be4375e Compare October 27, 2023 02:06
@Rustin170506 Rustin170506 marked this pull request as ready for review October 27, 2023 02:08
@Rustin170506 Rustin170506 changed the title WIP: Add fallback search sections to cargo remove Add better error message when it can not find the search section Oct 27, 2023
@Rustin170506
Copy link
Member Author

@epage Friendly ping~

@Rustin170506 Rustin170506 force-pushed the rustin-patch-remove branch 3 times, most recently from f166eea to 5ee48e7 Compare November 5, 2023 04:31
@epage
Copy link
Contributor

epage commented Nov 6, 2023

#12921 addresses the nightly failure

@Rustin170506
Search dep in other tables and update tests
fb0bbe3 
Signed-off-by: hi-rustin <rustin.liu@gmail.com>
@epage
Copy link
Contributor

epage commented Nov 7, 2023

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 7, 2023

📌 Commit fb0bbe3 has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2023
@bors
Copy link
Collaborator

bors commented Nov 7, 2023

⌛ Testing commit fb0bbe3 with merge d899b51...

@bors
Copy link
Collaborator

bors commented Nov 7, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing d899b51 to master...

@bors bors merged commit d899b51 into rust-lang:master Nov 7, 2023
20 checks passed
@weihanglo weihanglo mentioned this pull request Nov 8, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2023
@bors
Auto merge of rust-lang#117699 - weihanglo:update-cargo, r=weihanglo
7cc997d 
Update cargo

7 commits in 65e297d1ec0dee1a74800efe600b8dc163bcf5db..7046d992f9f32ba209a8079f662ebccf9da8de25
2023-11-03 20:56:31 +0000 to 2023-11-08 03:24:57 +0000
- fix: Report more detailed semver errors (rust-lang/cargo#12924)
- Fix some broken links in the man pages (rust-lang/cargo#12929)
- Add better error message when it can not find the search section (rust-lang/cargo#12865)
- Bug 12920 (rust-lang/cargo#12923)
- Update link in environment-variables.md (rust-lang/cargo#12922)
- refactor(toml): Pull out the schema (rust-lang/cargo#12911)
- tests: Remove plugin tests (rust-lang/cargo#12921)

r? ghost
@ehuss ehuss added this to the 1.75.0 milestone Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@weihanglo weihanglo

Labels
A-cli Area: Command-line interface, option parsing, etc. A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues Command-remove S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.75.0
Development

Successfully merging this pull request may close these issues.

cargo rm foo on error should suggest --dev or --build, if appropriate
6 participants
@Rustin170506 @rustbot @epage @bors @ehuss @weihanglo