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

Prototype for conversion to CSTParser.EXPR #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

c42f
Copy link
Member

@c42f c42f commented Mar 10, 2022

This demonstrates the basic approach working, but various things don't
work here yet due to at least two things:

  • Mismatch between the way the two packages tokenize the input, eg, for
    the delimiters in strings.
  • Missing "trivia nodes" in JuliaSyntax (eg, a brackets node). These
    should probably be added.

@c42f
Copy link
Member Author

c42f commented Mar 10, 2022

@pfitzseb here's an initial prototype of the CSTParser.EXPR conversion.

Currently it's still in the prototypes directory which I think is fine while experimenting.

However to make this usable but also avoid a CSTParser dependency we could either

  • Put this in a separate compatibility package.
  • Duplicate the CSTParser.EXPR data structure, in a JuliaSyntax.CSTParser compat module. But we'd also need to duplicate some CSTParser predicates and potentially the iteration code which could add up to a lot of code.

This demonstrates the basic approach working, but various things don't
work here yet due to at least two things:
* Mismatch between the way the two packages tokenize the input, eg, for
  the delimiters in strings.
* Missing "trivia nodes" in JuliaSyntax (eg, a brackets node). These
  should probably be added.
@davidanthoff
Copy link
Contributor

Another option would be that CSTParser.jl takes a dependency on JuliaSyntax.jl, and just starts to use the parser her under the hood? We might even be able to keep the complete CSTParser API unchanged and non-breaking?

@c42f
Copy link
Member Author

c42f commented Jun 21, 2022

Sure! If everyone maintaining CSTParser is happy to do that, I think that's an excellent solution!

@sjkelly
Copy link

sjkelly commented Aug 9, 2023

I suppose this would be even cleaner if CSTParser.jl incorporated this code as a package extension?

@davidanthoff
Copy link
Contributor

I think it would still be cleaner to go with my idea from above. We have had serious difficulty maintaining even just one parser in CSTParser due to a lack of maintainer time, so if we were to go with a package extension story, we would effectively have to maintain two parallel things at the same time, which would presumably require even more time. Plus, I don't really see any benefit of that?

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.

3 participants