search

tree-sitter-grammars / tree-sitter-markdown

Markdown grammar for tree-sitter
MIT License
375 stars 45 forks source link

Add wiki-links support (#54) #97

Closed boltlessengineer closed 1 year ago

boltlessengineer commented 1 year ago

Added basic wiki-link parsing based on Obsidian's wikilink.

MDeiml commented 1 year ago

Hey, thank you, this looks great. Not sure if it breaks in some edge cases, e.g. nested links, but that is not too important.

I would make it a "non-default" extension for the moment if that is ok with you? That would mean removing the EXTENSION_DEFAULT part in the line you added in common/grammar.js.

It also looks like you broke 2 test cases, see https://github.com/MDeiml/tree-sitter-markdown/actions/runs/4902920851/jobs/8755052765?pr=97. Could you have a look at that?

boltlessengineer commented 1 year ago

Hey, thank you, this looks great. Not sure if it breaks in some edge cases, e.g. nested links, but that is not too important.

Because Wiki-link parsing is an extension, prioritizing the wiki-link syntax in nested-link should solve the problem. Example 556 in corpus/spec.txt is showing some nested case.

I would make it a "non-default" extension for the moment if that is ok with you? That would mean removing the EXTENSION_DEFAULT part in the line you added in common/grammar.js.

I actually tried to make this non-default extension, but I couldn't figured out how to build & test as non-default extension. I learned tree-sitter syntax first time while making this PR. Can you give me an example of building & testing non-default extensions? That will be helpful.

It also looks like you broke 2 test cases, see https://github.com/MDeiml/tree-sitter-markdown/actions/runs/4902920851/jobs/8755052765?pr=97. Could you have a look at that?

Those two test cases should be broken when wiki-link option is on. That is intended behavior because Wiki-link is not in CommonMark/GFM spec. Should I remove those test cases?

MDeiml commented 1 year ago

Sorry for the late answer.

Can you give me an example of building & testing non-default extensions?

You have to set the environment variable corresponding to the extension you want to enable, e.g. EXTENSION_WIKI_LINK=1. How you do this depends on your operating system. On Linux and Mac you should be able to just write EXTENSION_WIKI_LINK=1 npm build, just be sure to run npm clean before that.

Should I remove those test cases?

Hm not to sure how to deal with this, I guess you can remove them for now and I'll think of something later.

boltlessengineer commented 1 year ago

@MDeiml I fixed points you said. Can you check this?

MDeiml commented 1 year ago

Thanks a lot, this is great! Build tests were just failing, because my CI used an older version of tree-sitter, but I just updated that to the newer version you were using.

MDeiml commented 1 year ago

Thanks for your work, I think a lot of people will be happy about this feature. Also sorry I took long with responding.