Skip to content

Add rustdoc team processes #852

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented May 21, 2025

cc @rust-lang/rustdoc

Rendered

@ehuss
Copy link
Contributor

ehuss commented May 21, 2025

Can you also add the rustdoc team to triagebot.toml for this directory and give yourselves permissions in https://github.com/rust-lang/team/blob/HEAD/repos/rust-lang/rust-forge.toml?

Copy link
Member

@aDotInTheVoid aDotInTheVoid left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I'm very happy to see this written down.

@fmease fmease added T-rustdoc Team: Rustdoc needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels May 21, 2025
@fmease
Copy link
Member

fmease commented May 21, 2025

(This PR will obviously need a T-rustdoc FCP to make it official once most things have been sufficiently fleshed out)

@fmease fmease added the S-waiting-on-team Status: Awaiting review/feedback/decision from relevant team(s) label May 21, 2025
Copy link
Member

@Manishearth Manishearth 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 seems pretty good!

@GuillaumeGomez
Copy link
Member Author

(This PR will obviously need a T-rustdoc FCP to make it official once most things have been sufficiently fleshed out)

Absolutely!

@GuillaumeGomez
Copy link
Member Author

I added the change in triagebot.toml as suggested by @ehuss.

I went through all comments.

@fmease: Hopefully I clarified that the rustdoc "groups" are not full-fledged teams and as such, to be part of them, you need to be a member of the rustdoc team.

Copy link
Contributor

@lolbinarycat lolbinarycat left a comment

Choose a reason for hiding this comment

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

a few nitpicks

Copy link
Contributor

@apiraino apiraino left a comment

Choose a reason for hiding this comment

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

left a comment after a cursory read.

Great work @GuillaumeGomez !

### Can I work on code experimentally before a approval is gained?
Of course! You are free to work on PRs or write code. But those PRs should be marked as
experimental and they should not land, nor should anyone be expected to review them (unless
folks want to).
Copy link
Contributor

Choose a reason for hiding this comment

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

or as draft.

Slightly tangential, there is an ongoing discussion about the overlap between the label S-experimental and the "draft" state that GH offers. I don't know if this discussion is relevant to this bit of documentation, just wanted to mention it.

Copy link
Member Author

Choose a reason for hiding this comment

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

No clue, might be worth revisiting want this discussion has a conclusion.

@GuillaumeGomez GuillaumeGomez force-pushed the rustdoc branch 2 times, most recently from 4da4ad9 to ab7ee18 Compare May 22, 2025 12:41
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

Haven't had a chance yet to review the whole thing in depth yet, but left comments on a few parts. Should have time within the next few days to finish reading though. Glad we're doing this!

If a member or maintainer has been inactive in rustdoc for 6 months, then we will ask them if
they would like to go to alumni status. If they respond yes or do not respond, they can be placed on
alumni status. If they would prefer to remain active, that is also fine, but they will get asked
again periodically if they continue to be inactive.
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that ideally we'd have some kind of policy where people get put on alumni involuntarily (after a longer period of time, perhaps) if they aren't contributing at all, even if they say they want to stay on the team. Of course, if they explain their circumstance then there could be exceptions. And also given how alumni works, they could always be brought back to the team if/when they can contribute again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think first establish such a policy, then amend the documentation.

If it helps doing things consistently across teams, T-compiler policy is the same: after X time you might be asked if you want/can continue to contribute. Then I imagine we could open a patch on the team repo with some wording like this and give the inactive contributor a chance to 👍 / 👎

Copy link
Member

@fmease fmease May 23, 2025

Choose a reason for hiding this comment

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

I think first establish such a policy, then amend the documentation.

I mean T-rustdoc doesn't have any policies at this point in time and this is the first ever document in which we formulate and thus make them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so this PR is the discussion about policies and it's not the result of a process happened in another place/time - correct?1

Footnotes

  1. sorry if it looks like I'm jumping in your garden, just trying to have consistent processes across teams

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely. And your input is very welcome as you have more experience with compiler team policy. ;)

@lolbinarycat
Copy link
Contributor

Kind of a nitpick, but should we do 10 days to match FCP length?

This is my first time hearing that's how long an FCP is, which is odd considering how many of my PRs have been through FCP.

I think there's a few causes for confusion:

  1. this isn't written down in a visible enough spot
  2. pFCP is a separate state which is even less documented, and does not seem to have an associated timeframe
  3. the rfcbot command that makes a PR enter pFCP is fcp

@GuillaumeGomez
Copy link
Member Author

Should be documented here I think. But yes, seems like it's missing. Sending them a PR.

@GuillaumeGomez
Copy link
Member Author

Opened rust-lang/rfcbot-rs#329.

@GuillaumeGomez
Copy link
Member Author

Applied @camelid's comments. Also, I updated the automatic alumni policy (did it in the second commit). Please take special attention to it and tell me if it looks ok to you. Maybe some extra thoughts from me about it: considering that any alumni can be brought back on the team on demand, I think it's pretty low barrier to keep it this way and they still are listed on the team's page (although in the alumnis list).

@lolbinarycat
Copy link
Contributor

We kinda say this under the fcp policy, but I think it would be good to explicitly say "bugfixes tend to get merged faster than user-visible changes" somewhere (maybe under "the path to membership"), as that's something that might alleviate some confusion/frustration for new contributors, and it's a fact that's not immediately obvious, as projects with less stability commitments may not give these types of contributions extra consideration the way the rust project does.

@lolbinarycat
Copy link
Contributor

I just realized code review is never mentioned anywhere in this. It probably should be.

@GuillaumeGomez
Copy link
Member Author

I just realized code review is never mentioned anywhere in this. It probably should be.

What do you think should be mentioned?


We use the Forge to document the team's processes, policies and working practices, if you'd like to
read about how the compiler and rustdoc work and instructions on how to set up a development environment,
you're looking for the [rustc-dev-guide](https://rustc-dev-guide.rust-lang.org/).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
you're looking for the [rustc-dev-guide](https://rustc-dev-guide.rust-lang.org/).
you're looking for the [rustc-dev-guide](https://rustc-dev-guide.rust-lang.org/rustdoc.html).

Comment on lines 46 to 50
It also implies some obligations (in some cases, optional obligations):

- Members may take part in various other [maintainer activities] to help the team.
- Members are held to a higher standard than ordinary folk when it comes to the [Code of
Conduct][CoC].
Copy link
Member

Choose a reason for hiding this comment

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

@Manishearth has said in the past something like "the minimum obligation for being a team member is responding promptly to FCPs" and i think that is a good rule that should end up in here.

(i don't have strong opinions on what "promptly" means but certainly i think it should be a month or less).

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jun 4, 2025

Choose a reason for hiding this comment

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

Good point, adding some numbers will be useful (and can be changed later on). I'll put one month for now.

Comment on lines 81 to 82
Being part of the front-end group doesn't change your rustdoc team membership. However
you will be assigned for reviews on front-end pull requests and on front-end FCPs.
Copy link
Member

@jyn514 jyn514 May 29, 2025

Choose a reason for hiding this comment

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

two thoughts about this:

  • i don't think being a team member should strongly imply that you have to do reviews (although certainly we can encourage it). again, i think the minimum is that people respond to FCPs. vice-versa, it seems fine for people on t-rustdoc to review frontend prs even if they're not on t-rustdoc-frontend.
  • "doesn't change your rustdoc team membership" is a little unclear - i think you meant "you don't have to be on t-rustdoc to be on t-rustdoc-frontend"? can you write that explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • i don't think being a team member should strongly imply that you have to do reviews (although certainly we can encourage it). again, i think the minimum is that people respond to FCPs. vice-versa, it seems fine for people on t-rustdoc to review frontend prs even if they're not on t-rustdoc-frontend.

Indeed, clarifying text.

  • "doesn't change your rustdoc team membership" is a little unclear - i think you meant "you don't have to be on t-rustdoc to be on t-rustdoc-frontend"? can you write that explicitly?

No I really meant that you need to be part of the rustdoc team to be able to have the t-rustdoc-frontend role. Gonna clarify it.

Copy link
Member

Choose a reason for hiding this comment

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

No I really meant that you need to be part of the rustdoc team to be able to have the t-rustdoc-frontend role.

oh, interesting. my impression was that these "roles" are so that people can be involved with one part of rustdoc without having to be involved with all parts. if we require someone to be on t-rustdoc to have a role, they'd still be responsible for all FCP's, not only the FCP's related to their role. Is that intentional?

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jun 4, 2025

Choose a reason for hiding this comment

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

It's exactly what the roles are for: by default, if you're part of no role, you're not part of any FCP. For now we only have the front-end role which allows to have FCPs with 4 people. I'd like to extend it to the other parts of rustdoc (ie JSON and "everything else").

Copy link
Member

Choose a reason for hiding this comment

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

oh, i see! that model makes sense to me :) but the transition confuses me: after this charter is merged, who will be responsible for "FCPs that aren't for the frontend"? you said people won't be on FCP's unless they opt in, which wouldn't leave anyone left to do "general" FCPs.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you're not part of any role, then you're not part of any FCP either, but that'd be strange, if someone is not part of any role, I don't think they should be on the team, or maybe there is a role missing and we can always add a new one if we think it makes sense. I listed 3 roles for the team:

  • front-end
  • JSON back-end
  • "everything else" (so that includes the HTML back-end, "cleaning" (rustc's to rustdoc's AST), rustdoc lints and more generally rustdoc internals)

Normally it covers all our current cases. If we want to make a general FCP, we can tag all the roles.

Copy link
Member

Choose a reason for hiding this comment

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

oh, i missed the "general" role. this all looks good to me then :) thanks for writing it up!

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem! Do you think it needs clarifications? If you misunderstood this part, others might as well but not sure which part led to this confusion.

Copy link
Member

Choose a reason for hiding this comment

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

i think i was just on mobile and didn't read enough of the context 😅

Comment on lines 72 to 75
## What proposal/approval do I need?
This section aims to exhaustively detail which proposal and approval is necessary for any given
circumstance.

Copy link
Member

Choose a reason for hiding this comment

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

this looks like a stub?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, removing it.

If there are additional resources which would be useful to a contributor or compiler team member,
feel free to submit a PR to add it here.

[dev_guide]: https://rustc-dev-guide.rust-lang.org/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[dev_guide]: https://rustc-dev-guide.rust-lang.org/
[dev_guide]: https://rustc-dev-guide.rust-lang.org/rustdoc.html

- Internals: The internals of rustdoc: interacting with the compiler, doctests, generating
rustdoc internal code representation, parsing command line arguments, lints, etc.

These groups are NOT full-fledge teams, and as such, to be part of any of these groups, you need to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
These groups are NOT full-fledge teams, and as such, to be part of any of these groups, you need to
These groups are NOT full-fledged teams, and as such, to be part of any of these groups, you need to

@lolbinarycat
Copy link
Contributor

What do you think should be mentioned [about code review]?

  1. One of the main responsibilities and privileges of team members is approving pull requests
  2. Code review from non members is welcome
  3. New features are usually subject to more in depth review, such as an FCP

@GuillaumeGomez
Copy link
Member Author

  • Clarified rustdoc team roles
  • Added more information about review policy
  • Fixed typos and links

Copy link
Contributor

@lolbinarycat lolbinarycat 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, just a few nits about phrasing

Comment on lines 81 to 82
Being part of the front-end group doesn't change your rustdoc team membership. However
you will be assigned for reviews on front-end pull requests and on front-end FCPs.
Copy link
Member

Choose a reason for hiding this comment

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

i think i was just on mobile and didn't read enough of the context 😅

@GuillaumeGomez
Copy link
Member Author

I think it's mostly ready now. I'm gonna switch it to full-fledged PR. I'll start the FCP in a few days if there is no new comment in-between.

@GuillaumeGomez GuillaumeGomez marked this pull request as ready for review June 5, 2025 08:54
@rustbot
Copy link
Collaborator

rustbot commented Jun 5, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 5, 2025
Add more information about review policy
Fix typos and links
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting review/feedback/decision from relevant team(s) T-rustdoc Team: Rustdoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.