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

Flexible grammar installer #54

Open
wants to merge 57 commits into
base: master
Choose a base branch
from

Conversation

Mandarancio
Copy link
Contributor

Instead of hard-coding the grammars and queries I added the possibilities to install them and add them in the init.lua user configuration file.

Now it is possible to configure its own grammar from local files or from remote git repositories (doc and examples in the README.md)

Copy link
Collaborator

@TorchedSammy TorchedSammy left a comment

Choose a reason for hiding this comment

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

ok so i did a quick check of this PR, and i have a few things to say.

firstly, i've been planning to add this feature eventually so thanks for deciding to work on it! the problem is that this removes "officially" supported queries and parsers. queries are taken from nvim-treesitter and are more complete than those hosted on the repository of a parser. if you could add this feature without removing officially supported parsers, that would be cool.

secondly, why did you remove the installer? the reason it downloads from the build repository is that windows users can't easily just compile a parser. additionally, downloading from somewhere it's already built is faster and more convenient.

thirdly, functions should be camelCase and commits should be conventional.

@TorchedSammy
Copy link
Collaborator

you could've used the existing installer functions to facilitate this feature, no?

i've also realized that this removes defaults from evergreen and users need to find the parsers to use for their languages. it adds extra effort for no good reason.

i would prefer to not remove the ease of use and only add the ability to configure extra parsers (which would be none in the future, optimally) and not all required ones of a user.

@Mandarancio
Copy link
Contributor Author

Mandarancio commented Oct 26, 2023

Hi @TorchedSammy, thank you for your feedback!
I did not think about Windows users (as I am not used to deal with them ;) ) but your concern makes completely sense!
I have the following proposal:

  1. Camel case function naming does not seem to be the standard as lite-xl-lsp or console use snake case, and these are officially supported plugin (or at least managed by the core team). So I believe this should be the convention (or at least for the functions that are called in the init.lua). For the commits I never heard about conventional and I will look into it. Maybe it can be useful to have some sort of guidelines about it in the project.
  2. I think I could have a solution on how to merge the simplicity and flexibility:
  • I will add another installation method that will download the pre-compiled library
  • I will add a structure (like yours) with some default languages and their configuration if the user adds a language without specifying any path/url/git the plugin will try to use the default config (if any)
  • I will add back the default queries as for the previous point if the user does not specify any queries parameter and an existing default query exists this one will be used
  • I will add a command to display the status of the plugin (installed grammars, file extension and so on)
  1. I do not think having a command install is a good idea as it hides too much information to the user (as it is finally a hidden configuration) and it makes more difficult to manage the grammars I think that add the default and simply add languages.add_grammar("c") to the init.lua is easy enough as well as very clear.

Let me know what do you think about my proposal.
Cheers!
Martino

@TorchedSammy
Copy link
Collaborator

  1. camel case is what i prefer for my code, so it will be what is used here.
  2. all the suggestions here sound good, but let's see how it'll turn out?
  3. i would prefer to keep the install command, no real reason to remove it. though other contributors can comment @xcb-xwii @takase1121

overall 👍

@Mandarancio
Copy link
Contributor Author

@xcb-xwii I do agree with you that something to manage parsers (at least to updates and clean the ones no more used) should be implemented (and I was planning to do so). For the moment at the initialization I just check if for the languages configured exists the parser library and the queries, if not I try to install them using the user configuration. But this is somehow similar to how it is done now (in the sense that evergeen does not check if there is a new version of the installed parsers or anything else, but it just installs the current version whenever the installation command is issued). Check automatically for updates could slow down the initialization of lite (in particular if many parsers are configured), so I propose to add an update and a clean command to do so.

@xcb-xwii, @TorchedSammy I added the following commands:

  • evergreen:update to update a single parser/queries
  • evergreen:update-all to update all parsers/queries
  • evergreen:clean to clean up unused parsers/queries
  • evergreen:status to check the available grammars

I do not think to implement any other features, as I think is now quite complete and flexible.
Best luck!

Copy link
Collaborator

@TorchedSammy TorchedSammy left a comment

Choose a reason for hiding this comment

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

lost a bit of interest in evergreen for a bit. still haven't tried out the changes myself. mainly cosmetic stuff

installer.lua Outdated Show resolved Hide resolved
installer.lua Outdated Show resolved Hide resolved
util.lua Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
extensions = { "cue" }, -- list of extensions to match.
-- it can be omitted if the language has the same name of the
-- extension (like in this case)
-- filenames = {...}, -- list of filenames to match
Copy link
Collaborator

Choose a reason for hiding this comment

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

better explanation? what are the filenames matching for..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file names are coming from the current implementation of evergreen and in particular at languages.lua:30.
But basically is for grammars that matches complete filenames and not only extensions, but I believe it would be better to add regular expression (such as %.cue) in order to first be more flexible (for example I have makesfile that could be matched by something like this Makefile.%) and avoid the need to have the two separate fields. Moreover I think lite-xl-lsp uses something like that.

What do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I updated it with a single filePatterns entry where it is possible to specify any regex (as in lite-xl-lsp:file_patterns (is really unfortunate to use different naming patterns...)).
Please let me know what do you think about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

assuming this is the same as ignore_patters in lite xl, it's a good change.

Copy link
Collaborator

@TorchedSammy TorchedSammy left a comment

Choose a reason for hiding this comment

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

that is all from me i think, @takase1121 @xcb-xwii review

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.

4 participants