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

Non-alpha characters included in ALPHA token (flow graph jison) #232

Closed
spect88 opened this issue Oct 23, 2015 · 3 comments
Closed

Non-alpha characters included in ALPHA token (flow graph jison) #232

spect88 opened this issue Oct 23, 2015 · 3 comments

Comments

@spect88
Copy link
Collaborator

spect88 commented Oct 23, 2015

Hi,

I've started looking into the problem of keywords not being valid suffixes of node ids (eg. monograph, upperclass, suspend) and there's a few things I've noticed.

The regex for ALPHA token matches lots of non-alpha characters like !"#$%&'*+,-.?.
See here: https://github.com/knsv/mermaid/blob/master/src/diagrams/flowchart/parser/flow.jison#L66
cc @codebeige

I guess these should be a different token, right? Some of them already have their token (COMMA, MULT, DOT), which works fine only because their definitions are above and the regex for ALPHA only matches a single character.

To fix the keyword suffixes problem, I thought I'd add a + to ALPHA token regex, so that it matches the whole string of alpha characters (eg. suspend) and keywords are only detected when ALPHA has ended. For that to work, meaningful characters like , would have to be excluded.

I've tried that and it seems to work (all tests pass), but I'm wondering if there were any reasons for ALPHA matching a single character and including punctuation.

Here's a small ruby script translating these regex ranges into human readable format: https://gist.github.com/spect88/da79e44bdd2c98e326d4

@knsv
Copy link
Collaborator

knsv commented Oct 24, 2015

That makes sense. I suspect that is a remainder from early days when text in nodes was ALPHA and NUM. Now, you would still want to be able to represent a diagram like the one below:

graph TB
   a.b-->b:d
   a.b-->c-d[e-f]

But alphaNumTokens are:

alphaNumToken  : ALPHA | NUM | COLON | COMMA | PLUS | EQUALS | MULT | DOT | BRKT ;

So that should not be a problem.

On that note, what do you think of a syntax for elipses somethinng like the suggestion below:

graph TB
   a(- Oval -)

Would you be interested in adding that as well?

@knsv
Copy link
Collaborator

knsv commented Oct 24, 2015

Also ... a heads up ... I have started to slowly move to es2015 syntax in mermaid. For instance multiline strings are quite convenient when setting up testtexts to parse. However ... it is suddenly crucial to run things in strict mode.

There is an issue in jison that makes the parsers not run in struct mode:
zaach/jison#285

as a result of that, if you work with the grammar:

  • make sure you have the latest version of the build environment, this uses the latest jison (unreleased)
  • use npm run jison to generate new parsers
  • use npm run jasmine to run the jasmine tests (using es6 features)

Good luck!

@knsv knsv closed this as completed Oct 24, 2015
@spect88
Copy link
Collaborator Author

spect88 commented Oct 24, 2015

OK, thanks for letting me know!

Tests in master are failing for me right now. Not sure if this is because I'm on node 0.10.38 or something changed in jison's master (probably not as last commit is from 12 days ago).
If node 4.0 is now required to build or run the tests, that's worth mentioning in the README I guess.
EDIT: Yep, the tests are passing now that I've switched to node v4.2.1.

Re ellipse syntax:
Yeah, once I'm done with keyword suffixes, I'll take a look at this.

mgenereu pushed a commit to mgenereu/mermaid that referenced this issue Jun 25, 2022
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

2 participants