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

Use rfcbot style for FCPs in the prioritization agenda #1784

Merged
merged 8 commits into from
Apr 23, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Mar 23, 2024

This PR add the necessary information to replicate the rfcbot style when showing FCPs and uses it in the prioritization agenda.

One thing at isn't perfect is that we are using the Github login and not the Zulip login, using the Zulip username would require accessing the team database which I didn't wanted to do yet but could be done as a follow up PR. (fixed, #1784 (comment))

r? @apiraino


They would now look like this (modulo the mentions). Full example here.

@Urgau
Copy link
Member Author

Urgau commented Mar 24, 2024

I've found a pretty neat trick. Zulip as a de-ambiguator syntax when pinging users with the same name, it works by specifying the Zulip ID after the name, like so @**Urgau|123456**; but if we omit the name and just provide the ID, like so @**|123456**, it will also ping the right person.

This greatly simplifies the process for us, as we don't have to query the name associated with the Zulip id.
I've added this trick in the last commit.

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.

hey @Urgau thanks for working on this! 🙂

I like the new layout you suggest for the FCPs.

I left a few comments (mostly nits), let me know what you think!

src/actions.rs Outdated Show resolved Hide resolved
src/github.rs Show resolved Hide resolved
templates/prioritization_agenda.tt Show resolved Hide resolved
@apiraino
Copy link
Contributor

apiraino commented Apr 2, 2024

Enabling mentions to remind team members checking their boxes in FCPs could have been a follow-up PR because it is unclear how it should work. However, for the sake of merging this change, I will approve this PR. For now it is disabled and will be discussed with T-compiler if and how that could work.

Thanks @Urgau for suggesting this formatting for the FCPs.

r=me

@apiraino
Copy link
Contributor

apiraino commented Apr 2, 2024

r? @jackh726 for the final word on this (I dont have permissions to approve)

Reverting my approval. I think this PR needs some more debugging

src/github.rs Outdated Show resolved Hide resolved
@apiraino
Copy link
Contributor

@jackh726 we're done here, I think. When you have a sec feel free to have a look (and ofc leave a comment). thanks!

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

One small nit

{%- if issue.fcp_details is object %}
{{indent}}- {{issue.fcp_details.disposition}}: [{{issue.title}} ({{issue.repo_name}}#{{issue.number}})]({{issue.fcp_details.bot_tracking_comment_html_url}})
{{indent}}{{indent}}-{% for reviewer in issue.fcp_details.pending_reviewers %} @{% if issue.fcp_details.should_mention %}{% else %}_{% endif %}**|{{reviewer.zulip_id}}**{%else%} no pending checkboxs{% endfor %}
{{indent}}{{indent}}-{% for concern in issue.fcp_details.concerns %} [{{concern.name}} (by {{concern.reviewer_login}})]({{concern.concern_url}}){%else%} no pending concerns{% endfor -%}
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to write something like concerns: ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, pushed the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

It now looks like this (modulo the pings):

@jackh726 jackh726 merged commit 808f9ff into rust-lang:master Apr 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants