-
Notifications
You must be signed in to change notification settings - Fork 1.9k
internal: migrate generate_delegate_trait to SyntaxEditor api
#21199
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
internal: migrate generate_delegate_trait to SyntaxEditor api
#21199
Conversation
5e796b9 to
d10843a
Compare
d10843a to
1a5d1bf
Compare
| @@ -254,51 +256,47 @@ fn generate_impl( | |||
| delegee: &Delegee, | |||
| edition: Edition, | |||
| ) -> Option<ast::Impl> { | |||
| let make = SyntaxFactory::without_mappings(); | |||
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.
Don't we need mapping here?
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.
This assist generates entirely new code (a new impl block) rather than editing existing syntax. Mappings are needed when using SourceChangeBuilder::make_editor() to edit existing syntax trees, where we need to track the relationship between original and new nodes for features like snippet placeholders. Since we're doing pure code generation, the mapping overhead is unnecessary.
Here is my understanding, is that right?
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.
Yeah, that's correct but given the long body of this assist, I'd prefer having a comment for that 😄
The changes for this migration are a bit more extensive than before, as some method calls indirectly use
ted::*apipart of #18285