-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Bug/4645 graph node containing keyword #4657
Bug/4645 graph node containing keyword #4657
Conversation
What this allows is for idStrings that are separated by dashes or underscores to be considered one whole string rather than a bunch of tokens mixed together. This is necessary for examples such as a-node-graph[text]. Now, the last part of the idString 'graph' will be read as part of the NODE_STRING token rather than attempting to add a GRAPH token to the idString.
This was never really used and had many things wrong with it. I believe that if a hex was provided in the diagram specification, the alphanum grammar would break it up into a BRKT and NUM token and use the first line with the addVertex() function. Second, the styleLink grammar provides the exact same functionality with the linkStyle keyword. Third, updateLink() expects an array of nums, not a hex digit. Fourth, no documentation is provided on this grammar statement existing. Fifth, the unit test does not work in version 10.2.4
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #4657 +/- ##
============================================
+ Coverage 45.32% 77.00% +31.68%
============================================
Files 52 144 +92
Lines 6645 14583 +7938
Branches 18 563 +545
============================================
+ Hits 3012 11230 +8218
+ Misses 3633 3243 -390
- Partials 0 110 +110
Flags with carried forward coverage won't be shown. Click here to find out more. |
@nirname, let me know what you think when you get the chance |
Similar to what was done in the class diagram parser, this will allow string tokens to appear in any state. This is especially helpful, because it will simplify the code and any future refactoring
Kinda immediate answer without looking into the code: |
Shouldn't this logic also apply to when there is an underscore? I've tested it and it works |
@nirname @sidharthv96, I thought of an additional edge case and was writing a unit test for it. Imagine we had a node with text such as I can do that of course, so in the end |
No. Quotes, if present, should wrap the entire string. The following seems to be a good path for us to follow. What do you think?
|
Originally, I thought this was necessary to prevent parsing the token as part of an edge. I forgot that the token will always be separated from the link/edge by the node id. Added an unit test for an edge case to be certain.
a52ac8e
to
834c67e
Compare
Supporting underscore is similar to variable names in most programming languages. We should keep things as-is here, and prohibit strange node names. |
This attempts to maintain the current behaviour. Previously, because HREF contained a space and called a state, the href token was able to be placed in the beginning of node ids (because it wouldn't conflict without the space). We require the space to keep that behaviour.
This will ensure that alphaNumToken does not lose any of the previously used tokens in its definition. The same tokens were added to textNoTagsToken explicitly, because it used to have alphaNumToken in its definition before I removed it. Previously, textNoTagsToken and alphaNumToken had many tokens in common in their definition. To avoid grammar conflicts, the alphaNumStatement grammar was created. However, I found this unintuitive and was an extra step just to avoid repetition in the definitions. I opted to have repetition in the definitions of textNoTagsToken and alphaNumToken and it be explicitly clear right away, rather than have extra grammar statements like alphaNumStatement which don't look like they do anything at first glance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Insightful job! Nicely done
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [mermaid](https://github.com/mermaid-js/mermaid) | [`10.3.0` -> `10.3.1`](https://renovatebot.com/diffs/npm/mermaid/10.3.0/10.3.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/mermaid/10.3.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/mermaid/10.3.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/mermaid/10.3.0/10.3.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/mermaid/10.3.0/10.3.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>mermaid-js/mermaid (mermaid)</summary> ### [`v10.3.1`](https://github.com/mermaid-js/mermaid/releases/tag/v10.3.1) [Compare Source](https://github.com/mermaid-js/mermaid/compare/v10.3.0...v10.3.1) #### What's Changed #### Bugfixes - fix style in contributors section of intro by [@​keer4n](https://github.com/keer4n) in [mermaid-js/mermaid#4670 - fix: [#​4676](https://github.com/mermaid-js/mermaid/issues/4676) redirect fix by [@​sidharthv96](https://github.com/sidharthv96) in [mermaid-js/mermaid#4693 - [#​2139](https://github.com/mermaid-js/mermaid/issues/2139) Applying user defined classes properly when calculating shape width by [@​knsv](https://github.com/knsv) in [mermaid-js/mermaid#4722 - Bug/4645 graph node containing keyword by [@​ibrahimWassouf](https://github.com/ibrahimWassouf) in [mermaid-js/mermaid#4657 - fix: Remove triple parsing of diagrams by [@​sidharthv96](https://github.com/sidharthv96) in [mermaid-js/mermaid#4697 - resolve info `HTML` and `Document` assignment by [@​Yokozuna59](https://github.com/Yokozuna59) in [mermaid-js/mermaid#4514 - fix!(deps): fix zenuml style leakage. by [@​danshuitaihejie](https://github.com/danshuitaihejie) in [mermaid-js/mermaid#4705 - Use our prettier config on the `packages/mermaid/src/config.type.ts` file by [@​aloisklink](https://github.com/aloisklink) in [mermaid-js/mermaid#4715 - create `ParserDefinition` type by [@​Yokozuna59](https://github.com/Yokozuna59) in [mermaid-js/mermaid#4719 - standardized `error` diagram by [@​Yokozuna59](https://github.com/Yokozuna59) in [mermaid-js/mermaid#4718 #### Documentation - Docs: Directives not needed in new diagrams as yaml formatter does this for all new diagrams by [@​Incognito](https://github.com/Incognito) in [mermaid-js/mermaid#4688 - Docs: add latest blog post by [@​huynhicode](https://github.com/huynhicode) in [mermaid-js/mermaid#4668 - Lychee config by [@​mmorel-35](https://github.com/mmorel-35) in [mermaid-js/mermaid#4699 - Syntax Update CONTRIBUTING.md by [@​soomrozaid](https://github.com/soomrozaid) in [mermaid-js/mermaid#4713 #### Chores - chore(deps): update all minor dependencies (minor) by [@​renovate](https://github.com/renovate) in [mermaid-js/mermaid#4682 - build(deps-dev): bump json5 from 2.2.1 to 2.2.3 by [@​dependabot](https://github.com/dependabot) in [mermaid-js/mermaid#4685 - build(deps): bump [@​braintree/sanitize-url](https://github.com/braintree/sanitize-url) from 6.0.0 to 6.0.1 by [@​dependabot](https://github.com/dependabot) in [mermaid-js/mermaid#4686 - build(deps-dev): bump vite from 4.3.3 to 4.3.9 by [@​dependabot](https://github.com/dependabot) in [mermaid-js/mermaid#4687 - chore(deps): update all patch dependencies (patch) by [@​renovate](https://github.com/renovate) in [mermaid-js/mermaid#4681 - chore: ts-ignore comment was misleading, JISON doesn't support types by [@​Incognito](https://github.com/Incognito) in [mermaid-js/mermaid#4689 - chore(deps): unpin the dompurify dependency by [@​djadmin](https://github.com/djadmin) in [mermaid-js/mermaid#4677 - build(deps-dev): bump pnpm from 8.3.1 to 8.6.8 by [@​dependabot](https://github.com/dependabot) in [mermaid-js/mermaid#4692 #### New Contributors - [@​keer4n](https://github.com/keer4n) made their first contribution in [mermaid-js/mermaid#4670 - [@​djadmin](https://github.com/djadmin) made their first contribution in [mermaid-js/mermaid#4677 - [@​danshuitaihejie](https://github.com/danshuitaihejie) made their first contribution in [mermaid-js/mermaid#4705 - [@​soomrozaid](https://github.com/soomrozaid) made their first contribution in [mermaid-js/mermaid#4713 **Full Changelog**: mermaid-js/mermaid@v10.3.0...v10.3.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/levaintech/contented). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi40MC4zIiwidXBkYXRlZEluVmVyIjoiMzYuNDMuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
📑 Summary
Fix issues relating to keywords being parsed in node idStrings, vertex text, and edge text
Resolves #4645
📏 Design Decisions
Previously, tokens were modularized to many parts. E.g., there was no Alphanumeric token, instead, there was an alphabet token
ALPHA
, a numbers tokenNUM
, and an underscore tokenUNDERSCORE
. The parser originally used recursion and concatenation on token types for things such as idString. However, this made it very difficult to debug issues such as thecall
andclick
keywords eliciting their respective states inside vertex and edge texts. Also, it was impossible to allow keywords in idStrings, because the parser would have to parse a keyword and then concatenate it to other tokens, resulting in eliciting errors for parsing a keyword in the wrong places.To combat this, I added separate states for vertex and edge text. Now, we can add anything between the vertices and links without adding additional token types to the grammar (doing this is what resulted in super long definitions such as
alphaNumToken
). We can also have quotes inside vertices and edges without escaping them.One problem I could not solve was allowing node idStrings to start with keywords. Previously, if the keyword occurred anywhere in the idString it would elicit an error. Now, it only elicits an error if it appears at the start of idString. E.g.,
graph-node
andgraph.node
would elicit an error. However, it's important to note that an error will not be elicited if the keyword is followed by an underscore. I.e.,graph_node
is valid. I'm not sure why this happens, as the regex is valid. I spent a couple days on this problem alone and could not make it work.📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.develop
branch