-
Notifications
You must be signed in to change notification settings - Fork 472
Add asserts in init methods #341
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
7762449 to
1a2c698
Compare
| % # some cases, like IdentifierToken, will add an empty string. | ||
| % # So if there is any empty string we just make it empty |
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.
If I understand your comment correctly, we currently set a Token’s text to "" if it essentially is a wildcard token and doesn’t have a fixed text, correct?
Since we now start relying on that behavior, I think it would be good if we document it in Token.py. E.g.
If `text` is not empty, a token of this type must always contain the specified `text`. If `text` is empty, the token can contain arbitrary text.
And while we’re at it, I think it would be nicer if we could use None instead of an empty string to denote the behavior that a token can contain arbitrary text – not sure what other changes that entails though.
Also a suggestion to make this comment clearer:
If a Token has an empty text, it can contain arbitrary text. If any of the token choices may contain arbitrary text, set assert_choices to [] so we don’t assert on the token’s text below.
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.
Yes the text in a Token is set to "" if it is None.
https://github.com/apple/swift/blob/f00f2841abf79b81a20b01936137eb095bd9b1df/utils/gyb_syntax_support/Token.py#L21
I agree that it would be more nice with an None instead of an "" in Token.
Added the comment above to Token.text
swiftlang/swift#40359
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.
I have run degyb locally where Token.text is None. Thus removed or "".
It didn't generate any other files?
Are there any other repos depending on it?
I could push and we could try to run CI?
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.
I’m not entirely if I understood your questions correctly, but I’m trying to answer as best as I can.
I have run degyb locally where
Token.textis None. Thus removedor "".
It didn't generate any other files?
It could very well be that changing Token.text from "" to None doesn’t change any of the generated files in the SwiftSyntax repo, so this might be expected.
Are there any other repos depending on it?
gyb_syntax_supportis only used by the compiler itself (ininclude/swift/Syntaxandlib/Syntax) and SwiftSyntax. So any breakage will be found by CI.
I could push and we could try to run CI?
Sounds good to me.
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.
CI did pass 🥳
8c7d7cd to
14f3604
Compare
14f3604 to
4f43f45
Compare
|
@swift-ci Please test |
ahoppen
left a comment
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.
A small comment nitpick. Otherwise 👍
4f43f45 to
266094c
Compare
266094c to
fa6658b
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
The idea is to add checks to help us and other developers to make valid swift code.
This only add for
text_choicesandtoken_choices.I have not figured out how to add for
node_choices, but this is a start.swiftlang/swift#40359