EmranMR / tree-sitter-blade

tree-sitter grammar for Laravel blade files
MIT License
186 stars 8 forks source link

Problem injecting php #5

Closed EmranMR closed 8 months ago

EmranMR commented 1 year ago

All editors have an issue injecting php. The problem arises because of the fact phpparser has injection query for html.

Temporary fix ideas:

calebdw commented 1 year ago

Instead of forking the repo and maintaining a separate copy of the original, the best thing to do would be to submit a PR to the original repo that splits into two different parsers, similar to the markdown parser in which two parsers live in the same repo:

image

EmranMR commented 1 year ago

Instead of forking the repo and maintaining a separate copy of the original, the best thing to do would be to submit a PR to the original repo that splits into two different parsers, similar to the markdown parser in which two parsers live in the same repo:

image

Very interesting @CalebDW. Quick question 🤔, that doesn’t look like a submodule as well, are they maintaining two separate folders?

PHP is ever growing so the main branch would be changing frequently as the new versions come out. Wouldn’t that mean duplicate work for writing new php grammars inside two separate folders?

Now what I had in mind in terms of maintainability (which might not even be practical, as I am not really an expert in GitHub/git) was the following:

1) fork the branch, do all the necessary adjustment, changing the parser name and somehow remove the html injection etc 2) The tree-sitter-php evolves on its own branch 3) whenever there is a new release we merge the main into the phpbase branch this way the “php only” version stays update to date by just a merge commit.

However that would mean either a separate forked project, or separate branch named phpbase/phpo in the main repo? But again as you mentioned not the most elegant solution.

I played around a bit with tree-sitter-php this morning to see if I can adjust, it became apparent easier said than done. I might get in touch in discord and see if I could ask for help from someone who has/had done work on the tree-sitter-php. They use a lot of external scanners and I am not sure how to adjust them for the new parser name. I get errors if I npm run build after changing the parser name. Narrowed it down to the way makefile figures the $PARSER_NAME and again not my forte. So hopefully I figure out how I can successfully generate and once done, I see if I can experiment with the grammar so that we can only inject php 🤞

calebdw commented 1 year ago

@EmranMR,

No those are not submodules, and the common grammar between the two is in a folder the next level up. In the case of the PHP parser I've already split them up and will try to submit a draft PR to garner feedback from the project maintainers.

There's no duplicate work, the entire php grammer is in the php-only directory and the php grammer just parses the text and php_only sections and then injects those languages into the nodes

EmranMR commented 1 year ago

@EmranMR,

No those are not submodules, and the common grammar between the two is in a folder the next level up. In the case of the PHP parser I've already split them up and will try to submit a draft PR to garner feedback from the project maintainers.

There's no duplicate work, the entire php grammer is in the php-only directory and the php grammer just parses the text and php_only sections and then injects those languages into the nodes

Oh wow, you have already done all the work + the PR draft!! 🎉🎉 Can’t wait to actually clone and try it out myself as well. Hopefully there are no issues and we get the merge soon 🤞 one of the roadblocks out the way, one step at a time.

stormherz commented 8 months ago

@calebdw sorry for bothering you about this. It seems like split parser support has landed (many thanks for that effort), but I wasn't able to verify it on my local setup (I'm the author of EmranMR/tree-sitter-blade#51).

Judging from the code of tree-sitter-blade, I'm assuming treesitter won't build php_only parser right away during :TSInstall, since Makefile still uses php parser by default, is this correct or am I missing the plot entirely?

calebdw commented 8 months ago

@stormherz no need to be sorry! There's an open PR to add the needed configs so that nvim-treesitter knows about php_only, but until that is merged you could always manually add it to treesitter

stormherz commented 8 months ago

@calebdw thanks! totally forgot about that option. works like a charm, actually

calebdw commented 8 months ago

php_only is now in nvim-treesitter so this issue can probably be closed

EmranMR commented 8 months ago

@calebdw Amazing! thank you again for sorting that out!