-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[SourceKit] Add subscript to doc structure (SR-5035) #11258
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
[SourceKit] Add subscript to doc structure (SR-5035) #11258
Conversation
@@ -146,6 +147,7 @@ struct SyntaxStructureNode { | |||
case SyntaxStructureKind::StaticVariable: | |||
case SyntaxStructureKind::ClassVariable: | |||
case SyntaxStructureKind::Parameter: | |||
case SyntaxStructureKind::Subscript: |
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 sure if this is the proper solution or if it's better to:
- Rename this function
- Whenever using
isVariable
also check ifKind == Subscript
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.
what happens if we don't add this line 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.
the syntax kinds of get
and set
inside subscript
s become identifier
instead of keyword
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 accessor (isVariable()
) is only being used in one place in the Syntax Model and only for things that have substructure that needs to be walked. Would it be OK to just rename it hasSubstructure()
or something?
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 agree with @CodaFi . Renaming it sounds a right approach 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.
Renamed and amended the commit.
cc @nkcsgexi |
84d1437
to
c4dad0c
Compare
@swift-ci please smoke test and merge |
This adds
subscript
to the structure reported by SourceKit.For example, for this file:
We get this structure now:
Resolves SR-5035.
This wouldn't be possible without the help from @johnfairh and @CodaFi. Thanks so much! 🙌