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

Replace zero-width whitespace with a visible \ in the PR template #131151

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Replace zero-width whitespace with a visible \ in the PR template #131151

merged 1 commit into from
Oct 2, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Oct 2, 2024 •
edited
Loading

People (incl. myself) tried to copy the r? which had an invisible zero-width whitespace (meaning it was actually more like r⌴?), causing rustbot to not recognize the instruction, and which makes it difficult to figure out why it didn't work.

This PR replaces the zero-width whitespace with a visible \ alongside a comment to remove the \. It's a bit less "visual", but at least it's visible why the rustbot assignment doesn't work.

@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2024

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 Oct 2, 2024
jieyouxu
jieyouxu commented Oct 2, 2024
View reviewed changes
.github/pull_request_template.md
@@ -7,6 +7,6 @@ tracking issue or there are none, feel free to ignore this.
This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

r​? <reviewer name>
Copy link
Member Author

@jieyouxu jieyouxu Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is actually

    r⌴? <reviewer name>

Can check with tools like https://www.dcode.fr/zero-width-space. This was initially added so the r/? in the template itself doesn't trigger rustbot, but it unintentionally makes it very confusing if someone tried to reuse this exact string.

fmease
fmease approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

one suggestion but otherwise r=me

.github/pull_request_template.md Outdated Show resolved Hide resolved
@jieyouxu
Replace zero-width whitespace with a visible \
aa3251d 
People tried to copy the `r?` which had an invisible zero-width
whitespace, causing rustbot to not recognize the instruction, and which
makes it difficult to figure out why it didn't work.
@jieyouxu jieyouxu force-pushed the adjust-pr-template branch from 04cac15 to aa3251d Compare October 2, 2024 11:11
@jieyouxu jieyouxu changed the title Replace zero-width whitespace with a visible / in the PR template Replace zero-width whitespace with a visible \ in the PR template Oct 2, 2024
@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 2, 2024

@bors r=@fmease rollup (this doesn't touch any code)

@bors
Copy link
Contributor

bors commented Oct 2, 2024

📌 Commit aa3251d has been approved by fmease

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 Oct 2, 2024
@fmease
Copy link
Member

fmease commented Oct 2, 2024

Testing that \ actually works and doesn't get erased by a markdown parser before rustbot gets the chance to look at it:

r? fmease

@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 2, 2024 •
edited
Loading

Wait a second, that also doesn't render visually? At least, one \ alone followed by ?. Would it be better if we used / (which exact character I really don't mind) so it gets visually rendered, like

r/? fmease

Or even

r⌴? fmease

to make it really obvious, lol.

@fmease
Copy link
Member

fmease commented Oct 2, 2024

Yeah, fair enough. I'm fine with / or ␣ then. I was worried that the string "r/?" looked so alien and might lead to a "what the f_ck" moment for new contributors.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 2, 2024
@fee1-dead
Copy link
Member

fee1-dead commented Oct 2, 2024 •
edited
Loading

Not sure why we'd need it to be visually rendered. The comment won't appear in the PR description anyways, and anyone that tries to copy from a rendered, escaped r? ghost would still copy the correct way to do it IIUC.

@jieyouxu
Copy link
Member Author

jieyouxu commented Oct 2, 2024

Fair enough. In that case

@bors r=@fmease

We can always re-adjust this template if there's any better methods.

@bors
Copy link
Contributor

bors commented Oct 2, 2024

📌 Commit aa3251d has been approved by fmease

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 2, 2024
@ehuss
Copy link
Contributor

ehuss commented Oct 2, 2024

We can also change triagebot to ignore html comments if that helps.

@matthiaskrgr matthiaskrgr mentioned this pull request Oct 2, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2024
@bors
Auto merge of rust-lang#131158 - matthiaskrgr:rollup-3x2vado, r=matth…
5384697 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#130863 (Relax a debug assertion for dyn principal *equality* in codegen)
 - rust-lang#131016 (Apple: Do not specify an SDK version in `rlib` object files)
 - rust-lang#131140 (Handle `rustc_hir_analysis` cases of `potential_query_instability` lint)
 - rust-lang#131141 (mpmc doctest: make sure main thread waits for child threads)
 - rust-lang#131150 (only query `params_in_repr` if def kind is adt)
 - rust-lang#131151 (Replace zero-width whitespace with a visible `\` in the PR template)
 - rust-lang#131152 (Improve const traits diagnostics for new desugaring)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 06d5218 into rust-lang:master Oct 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2024
@rust-timer
Unrolled build for rust-lang#131151
152a662 
Rollup merge of rust-lang#131151 - jieyouxu:adjust-pr-template, r=fmease

Replace zero-width whitespace with a visible `\` in the PR template

People (incl. myself) tried to copy the `r?` which had an invisible zero-width whitespace (meaning it was actually more like `r⌴?`), causing rustbot to not recognize the instruction, and which makes it difficult to figure out why it didn't work.

This PR replaces the zero-width whitespace with a visible `\` alongside a comment to remove the `\`. It's a bit less "visual", but at least it's visible why the rustbot assignment doesn't work.
@jieyouxu jieyouxu deleted the adjust-pr-template branch October 2, 2024 19:22
@apiraino apiraino mentioned this pull request Oct 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease approved these changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
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.83.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jieyouxu @rustbot @bors @fmease @fee1-dead @ehuss @Mark-Simulacrum