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: auto completion and linting for printer.cfg #1457

Conversation

forgodtosave
Copy link
Contributor

@forgodtosave forgodtosave commented Jul 10, 2023

Description

This PR adds the auto completion and linting for the printer.cfg file. It is based on the grammar for klipperCfg files. The Klipper Configuration Reference is used as the source of all possible configuration options. This is then parsed and used by the autocomletion and the linter respectively.

TODOs

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

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

@meteyou
Copy link
Member

meteyou commented Jul 11, 2023

I checked briefly this PR. I don't like the concept of a project within a project. So the second package.json + rollup to create a submodule. I prefer that you integrate this into Vite.

With your test library, I don't know how exactly you want to use it if the grammatic will only be created on the client.

Furthermore, it is just like described above. Because the .mocharc.json and all test files are in the @/plugins/code mirror, it looks like a project within a project. If using this test-lib for the finished feature makes sense, I prefer to use it in root and shorten the text-configs as much as possible because any line from the test config can be deprecated anytime.

@forgodtosave
Copy link
Contributor Author

I don't think I made it very clear that this is actually the branch from #1400 with an additional commit that then adds autocomletion and linting for the language defined there. So the discussion actually belongs in #1400.

But yeah, I also don't really like the project within a project. I just haven't found any other solution. Of course I could publish the grammar and the language separately and import them like the other codemirror languages. That would probably be the nicest thing, but since the language is really only useful for klipper, I think the language is better off only in mainsail.

If the language should stay in mainsail I'm not sure how you mean that I should integrate the package.json and rollup into vite.

Actually, I only need the package.json file so I can set the type to module so that the mocha tests work. I find the test particularly important here, especially if the grammar is ever changed again it happens really fast that you accidentally destroy some functionality. I also don't think that the tests will suddenly become outdated because they are based solely on the syntax of the klipper configuration. New possible parameters or the same are not a problem for the test. Of course, the test-configs from AlexZ can become outdated, should I load them dynamically? These are only tested to ensure that they can be parsed without errors, so you don't have to worry about the correct output being expected for new configs.

The tests for the linter are not as important. What is desired. test or no tests?

@meteyou
Copy link
Member

meteyou commented Jul 30, 2023

I meant that you should move mocha and the tests to the root directory.

But I need to find out how helpful the tests are (except for developing) because the parsing of the docs file should happen at the client (live system) rather than at/in the developer/release process.

@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