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

refactor: use macro to remove duplication of workspace inheritable fields getters #12317

Merged
merged 3 commits into from
Jun 29, 2023
Merged

refactor: use macro to remove duplication of workspace inheritable fields getters #12317

merged 3 commits into from
Jun 29, 2023

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

Use macros to avoid repeating the getter method definitions in workspace inheritable fields.

This also removes some unnecessary clones.

How should we test and review this PR?

I am not sure if it is preferable. Feel free to reject and close.

r? @Muscraft

@rustbot rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2023
@weihanglo
Copy link
Member Author 

 thread 'registry_auth::token_not_logged' panicked at 'assertion failed: `(left == right)`
  left: `8`,
 right: `7`', tests/testsuite/registry_auth.rs:588:5


failures:
    registry_auth::token_not_logged

Not sure why but I had a hard time reproducing it on a Linux machine.

@bors try

@bors
Copy link
Contributor

bors commented Jun 28, 2023

⌛ Trying commit 324af1d with merge 248bf89...

bors added a commit that referenced this pull request Jun 28, 2023
@bors
Auto merge of #12317 - weihanglo:inheritable-macro, r=<try>
248bf89 
refactor: use macro to remove duplication of workspace inheritable fields getters
@bors
Copy link
Contributor

bors commented Jun 28, 2023

💥 Test timed out

Muscraft
Muscraft reviewed Jun 28, 2023
View reviewed changes
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.

Overall this is a good change. One of my biggest regrets with workspace inheritance was not finding a way to reduce the amount of duplicate code. This is an excellent step in that direction!

My only concern would be with the potential drift in the doc comments for the getters as some are handwritten while the rest are generated by the macro. This is a very minor concern that doesn't need to be dealt with in currently, but it is something to note.

src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Show resolved Hide resolved
src/cargo/util/toml/mod.rs Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Outdated Show resolved Hide resolved
src/cargo/util/toml/mod.rs Show resolved Hide resolved
@weihanglo
Copy link
Member Author

Thanks for the feedback! It's ready for the next round of review :)

Muscraft
Muscraft approved these changes Jun 29, 2023
View reviewed changes
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.

Thanks!

@bors r+

@Muscraft
Copy link
Member

Not sure why bors didn't work, so I'll approve it again

@bors r+

@bors
Copy link
Contributor

bors commented Jun 29, 2023

📌 Commit e313b61 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 Jun 29, 2023
@bors
Copy link
Contributor

bors commented Jun 29, 2023

⌛ Testing commit e313b61 with merge b26b533...

@bors
Copy link
Contributor

bors commented Jun 29, 2023

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

@bors bors merged commit b26b533 into rust-lang:master Jun 29, 2023
@weihanglo weihanglo deleted the inheritable-macro branch June 29, 2023 21:08
@bors bors mentioned this pull request Jun 29, 2023
Closed
@weihanglo weihanglo mentioned this pull request Jul 1, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 1, 2023
@matthiaskrgr
Rollup merge of rust-lang#113227 - weihanglo:update-cargo, r=weihanglo
f67d59b 
Update cargo

6 commits in 03bc66b55c290324bd46eb22e369c8fae1908f91..5b377cece0e0dd0af686cf53ce4637d5d85c2a10
2023-06-23 23:27:46 +0000 to 2023-06-30 00:01:00 +0000
- Add READMEs for the credential helpers. (rust-lang/cargo#12322)
- Add some more documentation for Source download functions. (rust-lang/cargo#12319)
- Don't try to compile cargo-credential-gnome-secret on non-Linux platforms. (rust-lang/cargo#12321)
- refactor: use macro to remove duplication of workspace inheritable fields getters (rust-lang/cargo#12317)
- doc: should be `.cargo-ok` (rust-lang/cargo#12318)
- refactor(registry): extract and rearrange items to their own modules (rust-lang/cargo#12290)

r? `@ghost`
@ehuss ehuss added this to the 1.72.0 milestone Jul 2, 2023
@Be-ing Be-ing mentioned this pull request Jul 15, 2023
Closed
@weihanglo weihanglo mentioned this pull request Sep 6, 2023
Merged
@ehuss ehuss mentioned this pull request Sep 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Muscraft Muscraft Muscraft approved these changes

Assignees

@Muscraft Muscraft

Labels
A-manifest Area: Cargo.toml issues 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.72.0
Development

Successfully merging this pull request may close these issues.
5 participants
@weihanglo @bors @Muscraft @ehuss @rustbot