-
Notifications
You must be signed in to change notification settings - Fork 129
Add await
and Protocol
to the identifiers escaping rules
#88
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
Conversation
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.
Hi @denil-ct, thanks for another contribution!
Adding the two keywords is the quick fix and covers the original issue, so let's keep this PR to just that change, and please add to the unit tests to have some automated testing.
Could you break out the hex encoding experiment into a separate PR? I'd like us to discuss it more, but I don't want to block fixing #34 for it. Thanks! 🙏
Sure, that makes sense. |
You can add await as well in this PR, the change is well understood. The hex stuff is new and we'll want to think about the implications to existing code etc, thus the separate PR. |
532bc2f
to
1d6d5bf
Compare
@swift-server-bot add to allowlist |
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.
Looks good, thanks! I checked the unit tests and we already cover the "keyword" case, so no real need to list all the keywords there.
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.
@denil-ct thank you for once again for diving in 💪
I left a comment about Protocol
—not sure if we need it.
Additionally, would you mind adopting a PR title that would make a good entry in the release notes. For this PR, something like:
Add `await` to the identifiers escaping rules
await
and Protocol
to the identifiers escaping rules
Motivation
Resolves #34
Modifications
Protocol
to the keywords set.await
to the keywords set.Result
Protocol
andawait
as property names.Test Plan
Tested and verified for both names. They are properly renamed to
_Protocol
and_await