Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(GH-793) Add syntax folding #777
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
(GH-793) Add syntax folding #777
Changes from all commits
392f855
bd76cdb
77aa878
94fdf89
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
bool
defaults to false so the= false
shouldn't be needed I think. Sincebool
is a non-nullable typeThere 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.
IMO, in this case, being explicit makes it more obvious what's going on. I don't feel there's any maintenance cost with leaving this in.
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.
Works for 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.
Is this based on the VSCode type? Would it be better as an enum? We could extend that as easily as a string, but it's faster to check, less error prone and can't be
null
.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.
Given that the VSCode API can have this as
null
, a common convention in C# is to have aNone
value in the enum:(Or similar)
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.
See discussion above about strings and enums. This type is String (https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.foldingRange.ts#L94-L99)
With 3 (at this time) well known strings (https://github.com/Microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.foldingRange.ts#L51-L67)
I couldn't use an enum as C# enums are ints (well, ordinal) not strings. I later just used string constants as I only needed it in one place
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.
Is it because it's a JSON deserialisation thing? If so we should take a quick look at how we deserialise messages and see if we can use
[JsonConverter(typeof(StringEnumConverter))]
or something like that on the type. Being able to use enums would just be really nice 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.
In this case it's serialisation only. We don't receive this from a client.
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.
Ok, I'm good with how we do this. I looked into restructuring it to allow enums, but given that it took several changes in various places, I might save it to go with some other work I've been wanting to do.