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 generate_targets into separate module #11445

Merged
merged 3 commits into from
Dec 2, 2022

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented Nov 30, 2022

What does this PR try to resolve?

The generate_targets function is fairly complicated with an absurd number of parameters. This PR refactors the function into a TargetGenerator struct that represents the state of the generator, and reduces the amount of parameter-passing between the relevant functions. Additionally, the generate_targets function has been refactored into two smaller functions create_proposals and proposals_to_units. The docscrape-specific functionality from #11430 has been pulled out into a separate function, as promised.

How should we test and review this PR?

This PR does not change any functionality, so no new tests are added. It should be reviewed for code style.

r? @weihanglo

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 30, 2022
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.

Do you feel it better to name it UnitGenerator in order to avoid the term target being overloaded too much? (And relevant method name as well)

I am not sure, though 😅.

@willcrichton
Copy link
Contributor Author

Yeah I agree that unit is the better term to use here. I just renamed all the appropriate functions and data structures.

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.

Looks pretty good to me. I'd like to merge it. If anyone doesn't like this kind of change, please stop me.

src/doc/contrib/src/architecture/compilation.md Outdated Show resolved Hide resolved
src/cargo/ops/cargo_compile/unit_generator.rs Show resolved Hide resolved
src/cargo/ops/cargo_compile/unit_generator.rs Outdated Show resolved Hide resolved
@willcrichton
Copy link
Contributor Author

@weihanglo just fixed both your comments.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 2, 2022

📌 Commit ad201bc has been approved by weihanglo

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 Dec 2, 2022
@bors
Copy link
Collaborator

bors commented Dec 2, 2022

⌛ Testing commit ad201bc with merge f6e737b...

@bors
Copy link
Collaborator

bors commented Dec 2, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing f6e737b to master...

@bors bors merged commit f6e737b into rust-lang:master Dec 2, 2022
weihanglo added a commit to weihanglo/rust that referenced this pull request Dec 3, 2022
9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306
2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000
- Refactor generate_targets into separate module (rust-lang/cargo#11445)
- Improve file found in multiple build targets warning (rust-lang/cargo#11299)
- Error when precise without -p flag (rust-lang/cargo#11349)
- Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430)
- Aware of compression ratio for unpack size limit (rust-lang/cargo#11337)
- Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403)
- Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400)
- Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387)
- Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2022
Update cargo

9 commits in e027c4b5d25af2119b1956fac42863b9b3242744..f6e737b1e3386adb89333bf06a01f68a91ac5306
2022-11-25 19:44:46 +0000 to 2022-12-02 20:21:24 +0000
- Refactor generate_targets into separate module (rust-lang/cargo#11445)
- Improve file found in multiple build targets warning (rust-lang/cargo#11299)
- Error when precise without -p flag (rust-lang/cargo#11349)
- Improve strategy for selecting targets to be scraped for examples (rust-lang/cargo#11430)
- Aware of compression ratio for unpack size limit (rust-lang/cargo#11337)
- Add test for rustdoc-map generation when using sparse registries (rust-lang/cargo#11403)
- Add error message when `cargo fix` on an empty repo (rust-lang/cargo#11400)
- Store the sparse+ prefix in the SourceId for sparse registries (rust-lang/cargo#11387)
- Update documentation for -Zrustdoc-scrape-examples in the Cargo Book (rust-lang/cargo#11425)
@ehuss ehuss added this to the 1.67.0 milestone Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

5 participants