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

Tree-Sitter based parser. #261

Merged
merged 45 commits into from
Sep 18, 2024
Merged

Conversation

apozharski
Copy link
Collaborator

This PR implements an alternative to the pygments/textmate grammar parsers using tree-sitter and tree-sitter-matlab.

@joeced
Copy link
Collaborator

joeced commented Aug 13, 2024

Thanks for the contribution. I can review it better if the CI tests are running.
Please add the missing packages in setup.py to allow the tests to run.

@joeced
Copy link
Collaborator

joeced commented Aug 14, 2024

It looks like you are missing a commit from master branch:

  • 8b5e683
    This fixes the numerous error regarding "path".

@joeced
Copy link
Collaborator

joeced commented Aug 16, 2024

Good work so far! Impressed about the progress. Do you need any help getting tests passing?

@apozharski
Copy link
Collaborator Author

apozharski commented Aug 16, 2024

I have some more time this afternoon to hunt down the remaining few issues which shouldn't be that bad now that I have re-learned how to get pytest to give me the information I want. I will let you know then if there are any tests that are particularly thorny, though from looking at the remaining failures it looks like primarily small issues with the interface provided by Mat*Parser is a little different from what the documenters expect. (And a few bugs in the code that identifies whether a comment should be attached to a property or not).

@apozharski
Copy link
Collaborator Author

apozharski commented Aug 19, 2024

Alright it took longer than expected but it seems like all the tests are passing now. There will have to be some changes due to some modifications that the tree-sitter-matlab developer requested to the features/bugfixes we needed (acristoffers/tree-sitter-matlab#15, acristoffers/tree-sitter-matlab#16, acristoffers/tree-sitter-matlab#17, acristoffers/tree-sitter-matlab#18) but they should be very minor.

@@ -138,12 +138,19 @@ def test_classes(mod):
assert isinstance(cls, doc.MatClass)
assert cls.getter("__name__") == "ClassInheritHandle"
assert cls.getter("__module__") == "test_data"
assert cls.bases == ["handle", "my.super.Class"]
assert cls.bases == [("handle",), ("my", "super", "Class")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be changed back to the original. A list of fully-qualified names?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea this can absolutely be changed back. I don't think this broke anything and I was thinking it may be useful for later processing but now with fresh eyes I am unconvinced.

assert abc.bases == ["ClassInheritHandle", "ClassExample"]
assert abc.attrs == {"Abstract": True, "Sealed": True}
assert abc.bases == [("ClassInheritHandle",), ("ClassExample",)]
assert abc.attrs == {"Abstract": None, "Sealed": None}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the value of the class attribute not parsed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe in this case it was parsed correctly but the matlab code did not assign true and simply used Abstract on its own. I have not yet integrated the existing code for attribute defaults, but I will for consistency.

import re

# rpath = "../../../syscop/software/nosnoc/+nosnoc/Options.m"
rpath = "/home/anton/tools/matlabdomain/tests/test_data/ClassWithTrailingCommentAfterBases.m"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hard-coded paths are not great after release :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:) yep got rid of this. Was using it for quick and dirty testing.

@apozharski apozharski mentioned this pull request Sep 5, 2024
@apozharski apozharski force-pushed the tree-sitter-dev branch 2 times, most recently from c929b54 to 6d9405f Compare September 5, 2024 13:19
@apozharski apozharski changed the base branch from dev-textmate-grammar-for-parsing to master September 5, 2024 14:53
@apozharski
Copy link
Collaborator Author

Hello @Remi-Gau (also @joeced if you have a chance)! I'd love to have another pair of eyes go over this PR. I am pretty happy with the feature set (I realized this inadvertently addresses #265, see tests involving f_with_input_argument_block.m) and don't particularly want to bloat the diff any more than it already is, but I am open to feedback. Hopefully this would be the last round and we can focus on addressing the open issues.

table of contents, and can also be used within the
:py:meth:`_toc_entry_name` method.

This method must not be used outwith table of contents generation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo: outwith -> with out

@joeced
Copy link
Collaborator

joeced commented Sep 17, 2024

Hello @Remi-Gau (also @joeced if you have a chance)! I'd love to have another pair of eyes go over this PR. I am pretty happy with the feature set (I realized this inadvertently addresses #265, see tests involving f_with_input_argument_block.m) and don't particularly want to bloat the diff any more than it already is, but I am open to feedback. Hopefully this would be the last round and we can focus on addressing the open issues.

I looks really nice! I like that tree-sitter looks like it's giving way more structure than pygments. You have my approval :)

@apozharski apozharski merged commit 4cd5c51 into sphinx-contrib:master Sep 18, 2024
35 checks passed
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