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

Feature Request - tree-sitter support #33

Open
peterhoeg opened this issue Mar 6, 2023 · 9 comments
Open

Feature Request - tree-sitter support #33

peterhoeg opened this issue Mar 6, 2023 · 9 comments

Comments

@peterhoeg
Copy link

There is already tree-sitter grammar for beancount, so it would be great to get support for that. Whether or not that will be a supplementary mode to the existing regexp based mode doesn't really matter that much, but there seems to be a lot of momentum behind tree-sitter including in emacs upstream.

Ref: https://github.com/polarmutex/tree-sitter-beancount

@dnicolodi
Copy link
Collaborator

Why is this a feature request? Which feature would be enabled by switching the implementation of beancount-mode to use tree-sitter? Emacs with native support for tree-sitter has not been released yet and kinks in the tree-sitter support are still been ironed out. From the look of it so far, a beancount-mode based on tree-sitter would be implemented alongside the regexp based one as making the two implementation coexist would be too complex for very little gain.

A while ago, I had a look at Beancount grammar for tree-sitter you link to, and I was not positively impressed, see posts on the Beancount mailing list: the main issues are that it lacks support for understanding indentation, thus it cannot correctly parse the Beancount syntax, and the structure of the generated syntax tree is too close to the Yacc grammar it was derived from instead than offering logical nodes.

Before knowing of that effort I also wrote a tree-sitter implementation of the Beancount grammar. If there is going to be work in this repository to write an Emacs beancount-mode based on tree-sitter, it will almost certainly be based on that.

@peterhoeg
Copy link
Author

peterhoeg commented Mar 9, 2023 via email

@dnicolodi
Copy link
Collaborator

Do you realize that this sounds like you are asking people to do "hard work" (your words) for free because you decided that it is cool to use tree-sitter?

It also means we would be getting folding support as well as text/movement operations for free

You would get it for free only because someone puts in the hard work of coding it for you. All these functionalities are already available in the current beancount mode, and they are equivalently free to you as someone else already coded them.

Sure, upstream emacs doesn’t support it yet, but there is absolutely already support for tree-sitter:

So this hypothetical someone rewriting beancount-mode using a tree-sitter grammar, not only needs to do the work once targeting the emacs in-tree support for tree-sitter, but actually twice, also supporting the out-of-tree library.

I understand that there is some friction between the 2 competing implementations but I hope that can be worked out.

I don't understand why you describe providing constructive criticism (to which there has been no reply in the merit of the technical deficiencies highlighted) as "friction".

The community will definitely benefit from having a single, officially “blessed” implementation.

Why?

@peterhoeg
Copy link
Author

peterhoeg commented Mar 10, 2023 via email

@dnicolodi
Copy link
Collaborator

Well, I know what I meant and that is definitely not it. I’m neither asking nor telling anyone what to do.

You decided to title this issue "Feature Request". Any dictionary would tell you that a request is "the act or an instance of asking for something". Therefore, you are asking the maintainer of beancount-mode to do some work, and nowhere you offered to contribute to that work.

What I intended to communicate with “friction” was exactly that it seemed to have gone beyond merely a technical discussion and into personal territory.

Can you please provide a pointer to what you are referring to exactly? I don't remember any element of the discussion that was not purely on the technical merit of the two implementations. Maybe I missed it.

@dnicolodi
Copy link
Collaborator

Sure, but there are also existing issues presumably as a consequence of the existing regex based solution: #3

This is an issue in some code that is kept only because @blais, the original author of beancount-mode, may be still relying on it. That old code indeed uses some simple regular expressions that do not handle that well some corner cases. The functionalities that all other users of beancount-mode should use have no know issues of that kind. I've had on my todo list to rewrite these old functions in terms of the improved ones since a long time.

@dnicolodi
Copy link
Collaborator

While I fully admit that I find the work around recovering from syntax errors in TS very fascinating, “cool” has very little to do with this particular request.

As someone that does not contribute to the development of beancount-mode, why do you care if it is implemented using regular expressions, or a tree-sitter grammar, or anything else? I already asked you why this is issue is formulated as a feature request and you answered

It isn’t really about specific features per se - at least not at this point. It’s more about the fact that this is the direction in which emacs is moving and all the tooling around it due to increased semantic understanding.

So, it appears that the only motivation is that tree-sitter is "the direction in which emacs is moving", or in other words this is what is the current trend in Emacs, or in only one word, this is what is "cool" in the Emacs world right now. Again, a dictionary would tell you that the only meaning of "cool" that makes sense in this context is "conforming to the custom, fashion, or established mode".

@dustinfarris

This comment was marked as off-topic.

@blais
Copy link
Member

blais commented Jun 24, 2024

I don't have an opinion that matters much on this topic (I try to spend the minimum amount of time on beancount-mode honestly), but I think what could be done is that if someone really wants this, sending an implementation based on an LSP that we could add would be welcome here. It's a large change, and I think a good starting point would be a separate file in this repo.

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

No branches or pull requests

4 participants