-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Stabilize Ident::new_raw #59002
Stabilize Ident::new_raw #59002
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
There's some discussion about stabilization in the tracking issue and it looks like one of the main questions is about the name of the API ( In light of all that I'd personally be in favor of the current @rfcbot fcp merge |
Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:
Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
(Including T-Lang on this was proper as this exposes language features programmatically...) I agree that the name is fine; I would also be alright with I do however think that the documentation on the function is a bit underwhelming; In particular, the semantics of @rfcbot concern tests-and-clearer-docs |
ping @adnanademovic @alexcrichton re. my request for clearer docs and a list of tests ^--- |
Ping again for docs, tests so we can stabilize. |
@@ -812,7 +812,7 @@ impl Ident { | |||
} | |||
|
|||
/// Same as `Ident::new`, but creates a raw identifier (`r#ident`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more line:
/// In addition to general identifier validity requirements, raw identifiers cannot be created for keywords usable in path segments (`self`, `super` and others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That leaves me somewhat confused; what does "general identifier validity requirements" entail here? Surely Ident::new_raw
has fewer restrictions than Ident::new_raw
but your wording suggests it has more -- in particular, when will Ident::new_raw
panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"general identifier validity requirements" -> "general identifier validity requirements inherited from Ident::new
".
We can point to the reference or something that describes what is an identifier.
Surely Ident::new_raw has fewer restrictions than Ident::new_raw but your wording suggests it has more
It does have more restrictions.
in particular, when will Ident::new_raw panic?
"raw identifiers cannot be created for" -> "this function will panic on attempts to create"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does have more restrictions.
Ah! $ident
accepts keywords as well whereas it does not accept r#self
-- that's pretty counter-intuitive. I think amending the documentation of fn new
would be good as well to note the fact that keywords are allowed.
Tests that need to be added:
Existing tests:
|
@adnanademovic any updates on this? |
Ping from triage: |
Pinging from triage: Thank you. |
Hi, sorry for the lack of response. I was extremely busy. If the 3 tests are all it's needed, I'll try to do it in the next few days. I just have to go learn how to actually compile tests! |
(Checking @aturon's box, as he's no longer on the lang team.) |
ping from triage @adnanademovic, any updates on this? |
Closing this since it has been inactive for a long time. Thanks for the effort :) |
Tracking issue: rust-lang#54723 This is a continuation of PR rust-lang#59002
…r=petrochenkov Stabilize Ident::new_raw Tracking issue: rust-lang#54723 This is a continuation of PR rust-lang#59002
…r=petrochenkov Stabilize Ident::new_raw Tracking issue: rust-lang#54723 This is a continuation of PR rust-lang#59002
Tracking issue: #54723