Skip to content
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

Add support for inline comments #11

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jumarini
Copy link

Added inline comment support. This required a bit of cleanup of existing field definitions. Tested with VS Code 1.94.2.

Issue: #10

I don't see a hard requirement for this in any spec, but it's a convention that makes sense.
…inition to avoid seeing commas in arrays with comments at the end. Make comments a valid end-of-dictionary-value delimiter.
@thejustinwalsh
Copy link
Owner

Thanks for your contribution!

I want to get some unit tests in this repo to ensure contributions are not breaking previous syntax highlighting features, but I have not sat down and thought about it for too long.

How are you testing your changes, and what files have you tested your syntax highlighting enhancements with?

@jumarini
Copy link
Author

Nice to see you're still around. Unittests would be the right approach for the CI pipeline.
To test this PR, I'm visually verifying in Visual Studio Code using the Extension Development host, syntax coloring, editor token inspection, and a test file (likely not an exhaustive method). I haven't dug into how to programmatically obtain the textmate scope of each character.

# test nominal single line comment
#test without space
   #test with indent
   # test with indent and space
value: [1,2,3] #test no space
value: [1,2,3] # test space
value: [1,2,3]# should FAIL to recognize due to no space
multiline_array: [ # test after array
  {},{} # test after dictionary array entry
] #test end of array
multiline_array:
[ # existing limitation: brace on a new line is not recognized as begin token
  {},{} # test after dictionary array entry
] #test end of array
multiline_value: "a" "b" # test after multiple strings
   "c" # test after newline string
next_value: 1233545 # test after constant
string_value: "#test within quote" "this" # test after hash in quote
dict_value # test before dict value
{ # existing limitation: brace on a new line is not recognized as begin token
} # test on end token after newline start
dict_value { # test after dict start token on same line
  embed_1: "abc" # test after dict entry
  embed_2: 132535 # test after dict entry
} # test on end token after same-line start  
[optional value]: 10 # test after optional value
  #test between code
  # test successive lines
[test]: #{dict:"value"} # test commented value

@thejustinwalsh
Copy link
Owner

Perfect.

I'll take a peak around the space to see how others are unit testing textmate grammars, and work through some tests of at least some of the areas I was struggling with initially when releasing this.

If that is delaying the PR for too long I'll do a quick visual regression test then merge.

@jumarini
Copy link
Author

I've successfully automated the (comment-centric) unittests using the vscode-tmgrammar-test package. After adding to npm test scripts, they run and pass in my fork on commit. I can modify the PR to include these, or add it after this PR is merged.

@jumarini jumarini marked this pull request as draft November 1, 2024 18:00
@jumarini
Copy link
Author

jumarini commented Nov 1, 2024

@thejustinwalsh, it's been a few weeks. Have you been able to check unittesting?

@thejustinwalsh
Copy link
Owner

Hey, I’ve been busy hustling for a new role.

I plan to add the unit test feature you propose in this PR and a few core unit tests. Then, I’ll run your commit against the unit tests to ensure we don’t regress core highlighting features.

I’m not entirely happy with the nested object matching logic, so unit tests will help us refactor boldly.

I’ll add this to my Monday task list. Thanks for your patience.

@thejustinwalsh
Copy link
Owner

@jumarini check out these additional comments that are allowed between syntax elements...

value: -
  # comment
  2.0         # Valid: whitespace and comments between '-' and '2.0'.

https://protobuf.dev/reference/protobuf/textformat-spec/#parsing

@jumarini
Copy link
Author

jumarini commented Nov 5, 2024

That's awful - I'm really hoping that use is rare. I'd recommend pushing it to a new issue to address.

@thejustinwalsh
Copy link
Owner

Right! When I built this I had no spec, so I will lump it into what I think is turning into another refactor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants