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(job_queue): docs and move types around #11758

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

Refine module-level documentation for job_queue, and trying to move types to proper places.

  • job_queue.rs -> job_queue/mod.rs
  • job.rs -> job_queue/job.rs
  • JobState -> job_queue/job_state.rs

How should we test and review this PR?

Run cargo doc --document-private-items --no-deps --open to proofread the doc.

This shouldn't affect anything. Only one thing need to make sure it doesn't go wrong. The major logic of doit closure is moved into JobState::run_to_finish. It shouldn't change the behavior, though.

Additional information

Does anyone think it beneficial to have a graph explaining how job queue works? Something like this (from puma/puma)

https://bit.ly/2zwzhEK

@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2023

r? @ehuss

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

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2023
Also add two new methods:

- `JobState::new` to create new `JobState`
- `JobState::run_to_finish` is extracted from old [`doit`][1].

[1]: https://github.com/rust-lang/cargo/blob/7ddcf0fe3348b7e0f4ad4a730eab60a20638ef28/src/cargo/core/compiler/job_queue.rs#L1122-L1168
@epage
Copy link
Contributor

epage commented Feb 22, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 22, 2023

📌 Commit ff7b54d 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 Feb 22, 2023
@epage
Copy link
Contributor

epage commented Feb 22, 2023

Does anyone think it beneficial to have a graph explaining how job queue works? Something like this (from puma/puma)

Any thoughts on how we write the diagram? I know github has mermaid integration but I think rustdoc is pretty limited in what it can do with code fences.

@bors
Copy link
Collaborator

bors commented Feb 22, 2023

⌛ Testing commit ff7b54d with merge 9d5b32f...

@weihanglo
Copy link
Member Author

Does anyone think it beneficial to have a graph explaining how job queue works? Something like this (from puma/puma)

Any thoughts on how we write the diagram? I know github has mermaid integration but I think rustdoc is pretty limited in what it can do with code fences.

I am thinking about ASCII arts. They are great for terminal users. However, I think pictures like what I posted above are more helpful for normal users. Does it make sense to provide both? In terms of pictures, mermaid or other tools are all great to me if we can keep source code and generated pictures side by side with the Rust files.

@bors
Copy link
Collaborator

bors commented Feb 22, 2023

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

@bors bors merged commit 9d5b32f into rust-lang:master Feb 22, 2023
@weihanglo weihanglo deleted the jobqueue branch February 22, 2023 23:51
@bors bors mentioned this pull request Feb 22, 2023
@epage
Copy link
Contributor

epage commented Feb 23, 2023

Does anyone think it beneficial to have a graph explaining how job queue works? Something like this (from puma/puma)

Any thoughts on how we write the diagram? I know github has mermaid integration but I think rustdoc is pretty limited in what it can do with code fences.

I am thinking about ASCII arts. They are great for terminal users. However, I think pictures like what I posted above are more helpful for normal users. Does it make sense to provide both? In terms of pictures, mermaid or other tools are all great to me if we can keep source code and generated pictures side by side with the Rust files.

I've tended to avoid ASCII art because I didn't want to manage the exactness needed to get everything lined up (and staying aligned) but if you want to do it, go for it.

The challenge with anything else is then its not in with the code unless we had fancy code fence support, so I figure it isn't worth it.

weihanglo added a commit to weihanglo/rust that referenced this pull request Feb 23, 2023
15 commits in 17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6..9d5b32f503fc099c4064298465add14d4bce11e6
2023-02-17 19:45:09 +0000 to 2023-02-22 23:04:16 +0000

- refactor(job_queue): docs and move types around (rust-lang/cargo#11758)
- Scrub more of the test environment (rust-lang/cargo#11757)
- Make more reads of environment variables go through the `Config` (rust-lang/cargo#11754)
- Revert "Update curl-sys to use libcurl 7.88.1" (rust-lang/cargo#11755)
- use consistent case (rust-lang/cargo#11748)
- Switch some tests from `build` to `check` (rust-lang/cargo#11725)
- Fix typo in sparse-registry warning message (rust-lang/cargo#11753)
- reuse url encoding from `url` crate, don't use separate `percent-encoding` (rust-lang/cargo#11750)
- Read environment variables through `Config` instead of `std::env::var(_os)` (rust-lang/cargo#11727)
- Update curl-sys to use libcurl 7.88.1 (rust-lang/cargo#11749)
- mdman: update pretty_assertions to reduce deps (rust-lang/cargo#11747)
- Cleanup tests (rust-lang/cargo#11745)
- Enhance help texts of position args (rust-lang/cargo#11740)
- Fix typo (rust-lang/cargo#11741)
- Update comment about cargo-ok (rust-lang/cargo#11724)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2023
Update cargo

15 commits in 17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6..9d5b32f503fc099c4064298465add14d4bce11e6
2023-02-17 19:45:09 +0000 to 2023-02-22 23:04:16 +0000

- refactor(job_queue): docs and move types around (rust-lang/cargo#11758)
- Scrub more of the test environment (rust-lang/cargo#11757)
- Make more reads of environment variables go through the `Config` (rust-lang/cargo#11754)
- Revert "Update curl-sys to use libcurl 7.88.1" (rust-lang/cargo#11755)
- use consistent case (rust-lang/cargo#11748)
- Switch some tests from `build` to `check` (rust-lang/cargo#11725)
- Fix typo in sparse-registry warning message (rust-lang/cargo#11753)
- reuse url encoding from `url` crate, don't use separate `percent-encoding` (rust-lang/cargo#11750)
- Read environment variables through `Config` instead of `std::env::var(_os)` (rust-lang/cargo#11727)
- Update curl-sys to use libcurl 7.88.1 (rust-lang/cargo#11749)
- mdman: update pretty_assertions to reduce deps (rust-lang/cargo#11747)
- Cleanup tests (rust-lang/cargo#11745)
- Enhance help texts of position args (rust-lang/cargo#11740)
- Fix typo (rust-lang/cargo#11741)
- Update comment about cargo-ok (rust-lang/cargo#11724)
@ehuss ehuss added this to the 1.69.0 milestone Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-rebuild-detection Area: rebuild detection and fingerprinting 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.

5 participants