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

feat: klipper config-file language #1400

Closed

Conversation

forgodtosave
Copy link
Contributor

@forgodtosave forgodtosave commented May 27, 2023

Description

This PR adds a codemirror language packet. This includes a lezer parser for the printer.cfg file where everything related to gcode/jinja2 is forwarded to the old parser. Furthermore, code folding for the config blocks as well as syntax error highlighting is implemented. Also a basic example of auto completion was implemented.

Here the following question arose:
How context sensitive should autocomletion and linting be?
Should only options be suggested that are also available under the following config block?
Should all options that cannot exist in the context also be marked as errors?
Should only certain data types be available after certain options?

This all should be possible (see auto completion example) but you would have to store all this information somehow.
Hard coding would be possible, but you would have to keep it up to date.
Of course, it would be much nicer and more effective to automatically extract this information from this file when updating klipper. That should be possible for the most part, even if certain dependencies may not be easy to extract.
I could try that, the only question is whether and how I should proceed. In an extra PR, ...?

(part of other new PR)

I'm still unsure about the following things and would appreciate feedback and ideas:

  • file structure (I used codemirror and lezer as inspiration)
  • Code highlighting (I currently like that each data type has its own color)
  • I added a script in package.json to generate the parser which needs to be done whenever the grammar is edited. Should that stay, rename, ...?
  • The outsourcing works, but the external parser seems to have a problem getting only this part of the configuration file, at least the color coding is not correct.
  • the auto generated files throw an styling error, should I really let the prettier run over then?

Related Tickets & Documents

Solution for issue #1355

Mobile & Desktop Screenshots/Recordings

[optional] Are there any post-deployment tasks we need to perform?

@meteyou
Copy link
Member

meteyou commented May 28, 2023

hey! thx for your PR. i'm looking forward to seeing when this one is finished.

  • do you think it make sense to "convert/integrate" the gcode/jinja parser to lezer?
  • i am currently not sure if it makes sense to mark all "unrecognized entries for this block" as errors. because new klipper settings would then also be displayed as "incorrect/error" until they are integrated into mainsail. or even if klipper is older than the mainsail version, this would not match. it would be best to get all possible variables from klipper during initializing.

sry... i posted this to early... i read the full post now. i would "drop" the error message for this PR and add it in another PR. Mainsail itself should have access to this file from the current klipper version. (http://<ip or hostname>/server/files/docs/Config_Reference.md)

@meteyou
Copy link
Member

meteyou commented May 28, 2023

please add this also to the .prettierignore:

src/plugins/languages/dist
src/plugins/languages/KlipperConfigLanguage/*.d.ts

this should fix the PR check and i think its not necessary to "clean" these files.

@forgodtosave
Copy link
Contributor Author

forgodtosave commented May 28, 2023

i have now added these two lines to .prettierignore:

src/plugins/CodemirrorLanguages/KlipperConfigLanguage/parser/klipperConfigParser.terms.js
src/plugins/CodemirrorLanguages/KlipperConfigLanguage/parser/klipperConfigParser.js

The paths in your suggestion don't seem to fit, or should that be an idea for a new folder structure?

I have also removed the auto completion and will add it better with better error highlighting in an extra PR.
The current error highlighting should be fine because as long as the naming scheme of the future klipper config blocks/options does not change dramatically, there will be no syntax errors.

And yes I think a lezer grammar for jinja2/gcode is a good idea but I'm not very familiar with it. But I could try it. Is there any good documentation regarding jina2 in klipper or what is possible in klipper but not natively supported by jina2?

(http:///server/files/docs/Config_Reference.md)

thanks for the hint I've been looking for how to get this information

@forgodtosave forgodtosave marked this pull request as ready for review May 30, 2023 10:14
@meteyou
Copy link
Member

meteyou commented Jun 15, 2023

sry for the long delay...

  • Yes my paths could be wrong. I don't tested it. I just extract it from the prettier error message.
  • I think its a good idea to have a separate PR for the auto completion.
  • Klipper don't support all commands functions of Jinja. Only a part of it. The best way for testing, is to check AlexZ printer config: https://github.com/zellneralex/klipper_config. I think you will find all possible options in this config.
  • Would it be possible to shorten the paths a bit for language files? CodemirrorLanguages and KlipperConfigLanguage are really long names.

@forgodtosave forgodtosave marked this pull request as draft June 20, 2023 22:27
@forgodtosave
Copy link
Contributor Author

No problem

  • I shorten the paths to src/plugins/Codemirror/KlipperCfgLang/parser/klipperCfg.grammar is this ok?
  • I found also a very strange bug: right know it is not possible to start a file with a comment (the problem is probably caused by lezer and how I import the grammar/tokenizer) so I marked this PR as draft again

@meteyou
Copy link
Member

meteyou commented Jun 24, 2023

maybe you can also drop the "Codemirror" directory.

@forgodtosave
Copy link
Contributor Author

mh I don't know. I the new version there will be some files in Codemirror folder which are not specific to any language like printLezerTree for better debugging or the configuration for the parser testing framework. Tomorrow I will hopefully have the new version running and will push it then you can form your own opinion.

@meteyou
Copy link
Member

meteyou commented Jun 28, 2023

ah ok. sry i overlooked this. then it looks good to me.

with code folding for the configuration blocks
and with basic auto completion example
fix parsing errors:
- paths with "~"
- special characters in [respond]
- gcode also appear after "<string>_gcode:"
fixed package-lock

fixed prettier and eslint errors
in the case of merge conflicts with rebase, only the version from develop was used
@forgodtosave
Copy link
Contributor Author

Fixed some grammar bugs.
I've also added:

  • a few more value types that should make linting easier
  • some tests that should test most of the grammar rules
  • test whether a parsing error occurs with any of the configurations of AlexZ
  • code to codemirror.vue (commented out) that can be used to debug the grammar.

But I'm not sure if it wouldn't be better to only build the parser/language if changes were actually made to it, instead of doing it with every build.

@forgodtosave forgodtosave marked this pull request as ready for review July 1, 2023 23:41
@meteyou
Copy link
Member

meteyou commented Jan 1, 2024

@forgodtosave any updates here?

@meteyou
Copy link
Member

meteyou commented Feb 3, 2024

I am closing this PR because no response. Please create a new (up-to-date) PR if you want to continue working on this feature.

@meteyou meteyou closed this Feb 3, 2024
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