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.

Sign up for GitHub

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

Jump to bottom

Add explanatory comments to the LSP implementation #65

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Add explanatory comments to the LSP implementation #65

merged 2 commits into from
Dec 20, 2024

Conversation

david072
Copy link
Contributor

@david072 david072 commented Dec 18, 2024 •
edited
Loading

This adds some explanatory comments to the LSP implementation to help readability as requested by @soni801. However, I am not sure how to properly document this code. I would appreciate any feedback about sections that need more explaining, unnecessary/self-explanatory comments, etc.

@david072 david072 marked this pull request as draft December 18, 2024 20:17
@soni801
Copy link
Contributor

soni801 commented Dec 18, 2024

This helps a lot!

I just skimmed through the comments, and I have two (minor) requests:

  • Could document the tokenizer functions that aren't documented now?
  • Could you maybe add some more inline comments to the parse() and parseToolsField() functions? I find them quite hard to follow hehe

Other than that, really happy with this! :-)

@david072
Add explanatory comments to LSP implementation
03b9cdc
@david072
Improve handling of path arguments
ec29bdf 
The `start` and `rngmanip` statements both (might) take a path argument,
which needs to be handled in a special way due to how the script is
tokenized. We now have a unified method in the parser for accepting such
strings.

The method `acceptConsecutiveTokens()` will accept any number of tokens
that are adjacent to each other (i.e. the first token's end is the same
as the next token's start, due to the end being exclusive) and return
how many tokens were accepted. This makes it possible to accept
continuous strings (like e.g. paths), but reject anything separated by a
space.
@david072
Copy link
Contributor Author

Alright @soni801 I have added some more comments to parse() and parseToolsField() functions, as well as to the tokenizer's functions, and tried to simplify some code. I agree, the functions are very long and hard to understand, it might be possible to simplify them further in certain places or split them up into smaller functions.

I also included a little fix/refactor for path arguments (see start or rngmanip) that makes the code simpler and correctly handles paths after rngmanip. If you'd rather like that in a separate PR, let me know and I will try my best to bend the commit history to do that ;).

Hope this helps!

@david072 david072 marked this pull request as ready for review December 19, 2024 15:12
@ThisAMJ ThisAMJ merged commit d13a325 into p2sr:main Dec 20, 2024
1 check passed
@david072 david072 deleted the lsp-docs branch December 20, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@david072 @soni801 @ThisAMJ