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

Migration to ts-org parser 0.2.0. #215

Merged
merged 3 commits into from
Feb 19, 2022
Merged

Migration to ts-org parser 0.2.0. #215

merged 3 commits into from
Feb 19, 2022

Conversation

kristijanhusak
Copy link
Member

No description provided.

@kristijanhusak kristijanhusak requested a review from levouh February 5, 2022 22:12
@kristijanhusak
Copy link
Member Author

@levouh I would appreciate if you would give this a test. I changed some of your code regarding edit src block, tests are all passing so we should be ok. I generally need testing on this to make sure nothing is broken.

@kristijanhusak kristijanhusak force-pushed the ts-org-1.0.0 branch 4 times, most recently from cee39d5 to 1286ea9 Compare February 5, 2022 22:31
@levouh
Copy link
Contributor

levouh commented Feb 6, 2022

I would hope the tests would catch any glaring issues, but I will use this branch for now and let you know if I have any issues. You got an idea in mind for when you want to merge this?

@levouh
Copy link
Contributor

levouh commented Feb 6, 2022

I'm not super familiar with Treesitter grammars, nor the upstream parser, but what is the biggest difference between the version that was pinned and the existing main?

Looks like there might be some additional support for:

  • Each item token is now a expr, so we could potentially add highlights easier for things like org-priority-faces? That would require checking tokens in every item, but we already do this for "todo keywords" and it does not seem to be a huge issue, we are just doing it x2 if we do this.
  • Named timestamp groups (as you seem to have adapted to here)
  • In progress checkbox item highlights by default
  • Got rid of "markup" things like super/subscript (which means things like under_score are matched with org_code, which is nice)

Anything else notable @kristijanhusak?

@kristijanhusak
Copy link
Member Author

I would hope the tests would catch any glaring issues, but I will use this branch for now and let you know if I have any issues. You got an idea in mind for when you want to merge this?

Tests helped a bunch while doing this, especially for the edit src part, since I'm not fully familiar with it.
Regarding the release, I'm still trying to find a best way to release this without breaking everyone's functionality. Update requires updating the grammar, which people are not used to that much. I'm thinking about moving custom grammer configuration to the orgmode setup function. This would allow me to hardcode the versions and setup instructions would be simpler/smaller.

what is the biggest difference between the version that was pinned and the existing main?

There isn't any new functionality. I just made it compatible with new version, and I need to verify that everything is working.

Each item token is now a expr, so we could potentially add highlights easier for things like org-priority-faces

I'm open to any extensions once we merge this. We can take some ideas from here https://github.com/milisims/tree-sitter-org/blob/main/queries/highlights.scm

@kristijanhusak kristijanhusak force-pushed the ts-org-1.0.0 branch 11 times, most recently from 6bc180f to 53cbd79 Compare February 6, 2022 15:59
@levouh
Copy link
Contributor

levouh commented Feb 6, 2022

There isn't any new functionality. I just made it compatible with new version, and I need to verify that everything is working.

Gotcha.

I'm open to any extensions once we merge this

Yup, ordering there makes sense.

I'm thinking about moving custom grammer configuration to the orgmode setup function. This would allow me to hardcode the versions and setup instructions would be simpler/smaller.

For sure, this seems like the best route going forward, but people would have to be updated to this bit first. From that, would it be worth merging the "Treesitter via .setup(...)" bit first, separate from this branch? It can default to the currently default grammar, but show a deprecation warning once this version of the grammar is merged?

I'm still trying to find a best way to release this without breaking everyone's functionality

I think your idea is the easiest way to go for this particular issue, but some other notes of a similar flavor:

  • Cut versioned branches that people can update to that have these breaking changes. This would be a bit more inclusive to "general breaking changes" that aren't as easy to flag via configuration. Obviously people can already pin commits in their own configuration, but this might be a bit easier.
  • Add a "breaking changes" issue like core does that people can follow if they feel the need. This wouldn't stop issues from being created as not everyone would follow that issue, but it might help, or at least provide an easy place to link to for those issues on how to address it.

@kristijanhusak
Copy link
Member Author

For sure, this seems like the best route going forward, but people would have to be updated to this bit first

Istead of doing everything in the setup, I just introduced a custom function to load up a grammar https://github.com/nvim-orgmode/orgmode/pull/215/files#diff-83b45fa03268fbaf07c250824cf10ba2471882c14bdb7ad6e3d36355cdbd906cR65

This also allows me to do a ts grammar version check for now and let user know that they need to update.
Also, ts grammar should ideally be loaded before the nvim-treesitter setup so "ensure installed" works properly.

Cut versioned branches that people can update to that have these breaking changes

I will definitely do this. This version will probably go as 0.2, and going forward I'll make releases more frequently. We had discussion here around making this a norm for all the plugins, so I want to take a stab at it.

Add a "breaking changes" issue like core does that people can follow if they feel the need

Completely agree. Will do this immediately.

@kristijanhusak kristijanhusak force-pushed the ts-org-1.0.0 branch 2 times, most recently from d4a71e5 to 319ac6b Compare February 6, 2022 20:29
@kristijanhusak kristijanhusak merged commit 43f6fe0 into master Feb 19, 2022
@kristijanhusak kristijanhusak deleted the ts-org-1.0.0 branch May 9, 2022 20:07
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