-
Notifications
You must be signed in to change notification settings - Fork 440
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.text
is 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_support
is only used by the compiler itself (ininclude/swift/Syntax
andlib/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 |
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_choices
andtoken_choices
.I have not figured out how to add for
node_choices
, but this is a start.swiftlang/swift#40359