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

refactor: Use more serde_untagged #12581

Merged
merged 1 commit into from
Aug 31, 2023
Merged

refactor: Use more serde_untagged #12581

merged 1 commit into from
Aug 31, 2023

Conversation

epage
Copy link
Contributor

@epage epage commented Aug 28, 2023

What does this PR try to resolve?

This is a follow up to #12574 that replaces hand-implemented Visitors with serde_untagged because I felt it is cleaner code

Due to an error reporting limitation in serde_untagged, I'm not using
this for some MaybeWorkspace types because it makes the errors worse. See dtolnay/serde-untagged#2

I also held off on some config visitors because they seemed more
complicated and I didn't want to risk that code.

How should we test and review this PR?

The main caveat is we might not have tests for every single error case.

Additional information

I felt this does a good job of cleaning up the code and by using it
more, new uses are more likely to use it.

Due to an error reporting limitation in `serde_untagged`, I'm not using
this for some `MaybeWorkspace` types because it makes the errors worse.

I also held off on some config visitors because they seemed more
complicated and I didn't want to risk that code.
@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2023

r? @ehuss

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

@rustbot rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2023
@epage epage force-pushed the untagged branch 3 times, most recently from a27b68b to ad91a29 Compare August 31, 2023 15:40
@rustbot rustbot added the A-semver Area: semver specifications, version matching, etc. label Aug 31, 2023
@epage epage marked this pull request as ready for review August 31, 2023 15:40
@epage epage changed the title Untagged refactor: Use more serde_untagged Aug 31, 2023
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

This looks pretty good!

Copy link
Member

@Muscraft Muscraft left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Muscraft
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2023

📌 Commit 4da5fa5 has been approved by Muscraft

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 Aug 31, 2023
@bors
Copy link
Contributor

bors commented Aug 31, 2023

⌛ Testing commit 4da5fa5 with merge 5aca9af...

@bors
Copy link
Contributor

bors commented Aug 31, 2023

☀️ Test successful - checks-actions
Approved by: Muscraft
Pushing 5aca9af to master...

@bors bors merged commit 5aca9af into rust-lang:master Aug 31, 2023
18 checks passed
@epage epage deleted the untagged branch September 1, 2023 00:12
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 6, 2023
Update cargo

21 commits in 96fe1c9e1aecd8f57063e3753969bb6418fd2fd5..d14c85f4e6e7671673b1a1bc87231ff7164761e1
2023-08-29 20:10:34 +0000 to 2023-09-05 22:28:10 +0000
- fix(resolver): Make resolver behavior independent of package order (rust-lang/cargo#12602)
- cargo-credential: change serialization of cache expiration (rust-lang/cargo#12622)
- Update registry-web-api.md yank/unyank comments (rust-lang/cargo#12619)
- test: new options of debuginfo are no longer unstable (rust-lang/cargo#12618)
- use split_once for cleaner code (rust-lang/cargo#12615)
- stop using lazy_static (rust-lang/cargo#12616)
- doc: adjust all doc headings one level up (rust-lang/cargo#12595)
- chore(deps): update compatible (rust-lang/cargo#12609)
- chore(deps): update rust crate cargo_metadata to 0.17.0 (rust-lang/cargo#12610)
- Prepare for partial-version package specs (rust-lang/cargo#12591)
- refactor: Use more serde_untagged (rust-lang/cargo#12581)
- fix(cli): Help users know possible `--target` values (rust-lang/cargo#12607)
- Tab completion for --target uses rustup but fallsback to rustc (rust-lang/cargo#12606)
- Fewer temporary needless strings (rust-lang/cargo#12604)
- fix(help): Provide better commands heading for styling (rust-lang/cargo#12593)
- fix(update): Clarify meaning of --aggressive as --recursive (rust-lang/cargo#12544)
- docs(changelog): Clarify language for Cargo.lock policy (rust-lang/cargo#12601)
- fix typo: "default branch branch" -> "default branch" (rust-lang/cargo#12598)
- fix: add error for unsupported credential provider version (rust-lang/cargo#12590)
- fix(help): Explain --explain (rust-lang/cargo#12592)
- fix(help): Remove redundant information from new/init (rust-lang/cargo#12594)

r? ghost
epage added a commit to epage/cargo that referenced this pull request Sep 30, 2023
This was broken in rust-lang#12581
epage added a commit to epage/cargo that referenced this pull request Sep 30, 2023
Errors like this aren't too helpful
```
error: stderr did not match:
1   1     error: failed to parse manifest at `[..]`2   2     3   3     Caused by:4   4       TOML parse error at line 3, column 275   5         |6   6       3 |                 version = 17
  7         |                           ^8        -  invalid type: integer `1`, expected SemVer version    8    +  invalid type: integer `1`, expected a string or workspace
```

This was broken in rust-lang#12581
bors added a commit that referenced this pull request Sep 30, 2023
fix(test): Add back in newlines to diffs

Errors like this aren't too helpful
```
error: stderr did not match:
1   1     error: failed to parse manifest at `[..]`2   2     3   3     Caused by:4   4       TOML parse error at line 3, column 275   5         |6   6       3 |                 version = 17
  7         |                           ^8        -  invalid type: integer `1`, expected SemVer version    8    +  invalid type: integer `1`, expected a string or workspace
```

This was broken in #12581
@ehuss ehuss added this to the 1.74.0 milestone Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues A-semver Area: semver specifications, version matching, etc. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants