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 parser support for backslash continuation #1180

Merged

Conversation

TheAfroOfDoom
Copy link
Contributor

@TheAfroOfDoom TheAfroOfDoom commented May 19, 2024

Summary

closes #1069

this PR adds backslash continuation for command and argument parsers in mcfunctions.

Preview

command parser argument parser (+ node range highlighting correctly)
image image
command and argument parser together correct index mappings across lines
image image
command and argument completers not working yet ...but completers do work as long as you're not splitting the node (?) up between lines?
completer not working summon completer sorta working entity selectors

Next steps

(these should be ticketized)

  • fix colorizer not really working across lines (e.g. open/close brackets [/] across lines)
    • plenty of examples of this in the above images
  • fix command completer mostly not working

@TheAfroOfDoom TheAfroOfDoom requested review from SPGoding and misode May 19, 2024 03:27
@SPGoding
Copy link
Member

amazing work! i'll look over this when i get more time.

open/close brackets [/] across lines

i believe that is not an us issue. we don't colorize brackets. do you have some other extensions?

Copy link
Member

@SPGoding SPGoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the completion is computed correctly but VS Code doesn't display them.

completion computed in log

Old information from when I was trouble shooting

i think this happens because VS Code uses text in the `replace` range to filter through the options, which does not match any of the `label` of the completion items. e.g., the `replace` range is `[(8, 0); (9, 3))`, so the source text VS Code uses to filter options is `ad\huh`. the `label` of the completion item is `advancement`, which does not match the source text, so no completion item is shown. one way i can think of to fix this is to add `filterText` like `ad\vancement` to the completion items so VS Code can filter the options properly (and if we can construct those `filterText` might as well set the `newText` to them so the user gets to keep their insane word-breaking line continuity). another way i can think of to fix it is to just set the `filterText` to an empty string so no filtering is done by the client, and we could do filtering on server side given that we have more information.

nvm, tried to hard code some filterText values and none of these work. i even tried to make filterText exactly the text in the replace range and it still doesn't work

image

image


in microsoft/vscode there's this comment explaining how filtering works. my understanding was mostly correct, except that VS Code does not support ranges that span multiple lines as the "prefix". a hacky workaround i can think of would be to make the replace range in textEdit stop at the end of the first line, then use additionalTextEdits to delete the following lines.

packages/core/src/parser/util.ts Outdated Show resolved Hide resolved
- concatenates lines together when we hit a trailing backslash
- `terminators` param is used differently for `argument`/`command` parser
  - difference is `command` parser adds in a space terminator ` ` in addition to newline terminators `\n` and `\r`

- also add unit tests :]
- add space char ` ` as a terminator to improve performance (needed to pass test)
- we want these to result in errors, but they pass right now
@TheAfroOfDoom TheAfroOfDoom force-pushed the 1069-support-backslash-continuation branch from 5b41b76 to 059a825 Compare May 20, 2024 15:59
@TheAfroOfDoom TheAfroOfDoom requested a review from SPGoding May 20, 2024 15:59
@SPGoding
Copy link
Member

Would it make sense to wrap concatOnTrailingBackslash on the entire command parser, instead of on individual argument parsers? With the current implementation the built-in parsers in mcfunction package (trailing content and unknown parser) does not support backslash continuation.

For something like this,

advancement ! \
aartsarst

The ! is unparsable, so anything after it is considered to be trailing content. The expected trailing content should expand into the second line, but with the current implementation the second line is treated as the start of a new command.

image

@TheAfroOfDoom
Copy link
Contributor Author

TheAfroOfDoom commented May 20, 2024

Would it make sense to wrap concatOnTrailingBackslash on the entire command parser

i believe it already was wrapped around the command parser:

https://github.com/TheAfroOfDoom/Spyglass/blob/059a825c4ea4e799b9ed2ebb17929e492d9a2b33/packages/mcfunction/src/parser/command.ts#L96-L103

however...

The ! is unparsable, so anything after it is considered to be trailing content. The expected trailing content should expand into the second line, but with the current implementation the second line is treated as the start of a new command.

good catch. when looking into where exactly we should place the concat parser i was reminded of our comment and command_macro parsing that is done in the mcfunction entry parser

this made me wonder how Minecraft would parse something like this:

# a comment \
say i will not say

turns out it prints nothing, meaning the backslash continuation is the first thing that happens when Minecraft parses a .mcfunction file (even prior to comment parsing). this isn't too crazy but was interesting to me nonetheless

the fix for this is fairly simple and actually lets me simplify some code/remove unnecessary test code. commits soonTM


preview

image

- will be used in next commit for new tests
- the main behaviors we want to test are:
  - comment detection
  - macro detection
  - splitting multiple lines into multiple commands/comments/macros accordingly
- output wrong thing right now, fixed next commit
- no terminators specified since this wrapper parser runs once at the start of mcfunction file parsing
- need to remove backslash cont. parser from internal `command` parser for these tests to pass, so that's in this commit too
  - remove command parser specific tests around backslash continuation since its handled by mcfunction entry parser earlier anyway
- when you're debugging these, it's not unlikely you'd be in a test longer than 30s (the default timeout)
- new default is 999999s (11.5 days)
- no child parsers should really directly use the new `concat` parser
- remove tests specific to backslash continuation
- it never stops at specific terminators other than end of string since its called on entry now
@TheAfroOfDoom TheAfroOfDoom force-pushed the 1069-support-backslash-continuation branch from 1f0c88b to 7f63ab2 Compare May 20, 2024 23:58
.vscode/launch.json Outdated Show resolved Hide resolved
packages/core/src/parser/util.ts Outdated Show resolved Hide resolved
packages/core/src/parser/util.ts Outdated Show resolved Hide resolved
packages/mcfunction/test/parser/utils.ts Outdated Show resolved Hide resolved
packages/mcfunction/test/parser/entry.spec.ts Outdated Show resolved Hide resolved
packages/core/src/parser/util.ts Outdated Show resolved Hide resolved
packages/mcfunction/test/parser/utils.ts Outdated Show resolved Hide resolved
@TheAfroOfDoom TheAfroOfDoom requested a review from misode May 21, 2024 00:58
@TheAfroOfDoom
Copy link
Contributor Author

TheAfroOfDoom commented May 21, 2024

still need to fix build

fixed in 67eab68 (#1180)

- no more java-edition dependency
- it was pretty easy idk why i didn't think of this before
@TheAfroOfDoom TheAfroOfDoom requested a review from SPGoding May 21, 2024 15:19
@TheAfroOfDoom
Copy link
Contributor Author

apparently i have no idea how to cleanly do a merge commit RIP

@TheAfroOfDoom TheAfroOfDoom force-pushed the 1069-support-backslash-continuation branch from ed91f57 to c16a08a Compare May 24, 2024 02:25
- it doesn't count as end of file iff there is any content after the newline after the backslash (including spaces), so catch that in an explicit case
@TheAfroOfDoom TheAfroOfDoom force-pushed the 1069-support-backslash-continuation branch from f6c0638 to 0548d94 Compare May 24, 2024 02:33
@TheAfroOfDoom TheAfroOfDoom force-pushed the 1069-support-backslash-continuation branch 2 times, most recently from fe4db37 to 0548d94 Compare May 24, 2024 02:49
@TheAfroOfDoom TheAfroOfDoom requested a review from misode May 25, 2024 17:24
- they have errors now, so we're gonna remove those in next commit
@misode misode merged commit 2420130 into SpyglassMC:main May 26, 2024
3 checks passed
@TheAfroOfDoom TheAfroOfDoom deleted the 1069-support-backslash-continuation branch May 26, 2024 20:21
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.

Support backslash continuation in functions
5 participants