-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(parser): reimplement parser in langium
instead of jison
#4401
Comments
This is exciting @Yokozuna59 ! I'd love to see the existing parsers modernised! |
Sure! I'm currently planning to support the pie chart since it's the easiest one. But I think I should support the common syntax and configuration first: comments, directives, accessible titles and descriptions, etc. I've got to improve the current comments and directives regexes since they aren't covering some weird cases; e.g., in #1645, if text has been wrapped inside classDiagram
class Cat {
+int age %% not skipped comment
}
|
@sidharthv96 It doesn't look like Langium or Chevrotain are customizable enough in our use case. I was trying to match the diagram title (and accessible title and description in the same way), so I wanted to match everything after |
I think I was able to do that in chevrotain. |
Although not in the same way, I was able to do it too. I've finished |
I think Can some verify I haven't forgotten anything? link |
Hmmmm, this is kinda a hack, so sticking it langium is probably the ideal way of doing this, but if you're feeling lazy, I wonder if it's easier to do this a pre-proccess step, e.g. something like: const diagram = `%%{init: { **insert argument here**}}%%
graph TD
A-->B`;
const directiveRegex = /%%(\{[\w\W]*\})%%/gmU; // use U-flag to make `*` lazy
const directives = diagram.matchAll(directiveRegex);
const diagramWithoutDirectives = diagram.replace(directiveRegex, "");
const parsedDiagram = yourLangiumParser.parse(diagramWithoutDirectives); It seems like directives are more like a metadata thing that applies to all diagrams, rather than something part of each diagram, so it might make sense to split it away from the diagram parser 🤷 Edit: Just thought of something else, all mermaid diagram definitions support using a YAML front-matter before the diagram, e.g. like: ```mermaid
---
# some YAML here
title: "Hi"
---
pie
"Rats" : 15
``` The way this is currently implemented is by doing a pre-processing step on all diagrams, so doing the same thing with directives wouldn't be too weird.
From a quick look, it looks great! Great work 👍! The |
I agree with @aloisklink, yaml and init directives can be handled externally for now as they aren't super important for our core usecase which is formatting and parsing the actual diagrams. |
I can create a general directive parser (still not easy), but it won't be strictly typed. So it won't suggest anything if the user tries to write some directives, unlike the strictly which would suggest according to what the user has typed using LSP. For example:
I tried tbh, but it would hard to preform with my experience with langium. It can be removed using hidden terminal rule:
Not really sure tbh, but I won't think about it for now and will focus on other stuff.
I think it won't be that hard to implement this since it's wrapped differently from comments (%%) and only appears at the top of the file, but I'm not really sure what possible values can be there since I couldn't find them in the docs.
I was trying to create a general regex that make sure that there is no |
I figured out the solution for those |
@Yokozuna59 will be developing this package in their repo, and we'll move it into mermaid once it's stable for production use. This will replace mermaid's current JISON parser once the team decides. I wonder if there's any way to verify that the new parser is backwards compatible with the syntax supported by the old parser? |
I'm currently creating unit tests for common stuff, for example, title, accTitle, accDescr, etc. And will add unit tests for each diagram. Currently, only pie chart and the above-listed common stuff have test cases. And I add more test cases for each edge case and valid syntax while developing. I could take the test cases for the current parser and add them to mine to validate that they're working fine. What do you suggest? |
Should I support multiline comments #1281 with it? I came across it while a ago. Please mention me for similar stuff if you could since there isn't a |
@Yokozuna59, let's keep your implementation as close to the current parser as possible. We can add new features and fix bugs once we make the switch. Otherwise it'll be difficult to test. Regarding testing & verification, unit tests and visual tests will definitely be required and helpful, but they won't cover all the usecases. Two ideas I had (which seems a bit over engineered) were,
Anything else? |
If your pie implementation is ready, can you raise a PR like #3432, so we can test the size increase? |
Okay, I'll remove the multi line comment for now.
I'm not really sure how I can help with that. Before implementing the parser in |
The Langium PR haven't been merged yet, so the title and accessibilities would give wrong result: - awesome title
+ title awesome title There is an alternative way to implement it; thanks to @msujew for pointing it out, I'll implement it then create a PR. |
@Yokozuna59 you can focus on langium for now. Don't worry about the verification part. I was sharing that to get input from the community. |
This comment was marked as resolved.
This comment was marked as resolved.
Never mind, all of the above issues were solved in I guess |
@sidharthv96 I would like to convert the Git Graph parser to use Langium, can I tag along this issue or should I create a new one? Perhaps we can add a checklist of the remaining parsers and make this an epic of sorts |
@NicolasCwy you can start with a draft PR directly, no need to create an issue. :) The flow what we've seen till now is usually Stage 1
Stage 2
Stage 1 and 2 can be separate PRs, so we can get stuff in easily. Few reference PRs |
From this conversion. It would be great to reimplement the parser in langium instead of jison the following reasons:
And much more!
I'm currently working to create a separate package using it here Yokozuna59/mermaid-lint. Anyone is welcome to contribute and help me! Even thoughts would be fine.
Here is an example I've built using Langium for pie charts (toggle the icon next to the link to show the AST). playground
related issues:
The text was updated successfully, but these errors were encountered: