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

Bug/1478 er diagram unicode support #5146

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Mikek16
Copy link
Contributor

@Mikek16 Mikek16 commented Dec 14, 2023

📑 Summary

Resolves #1478

📏 Design Decisions

add literal unicode character range to ALPHANUM regex

Copy link

netlify bot commented Dec 14, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit a656550
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/657b4270eff48e00085de7df
😎 Deploy Preview https://deploy-preview-5146--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Dec 14, 2023
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Merging #5146 (a656550) into develop (6e64556) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5146   +/-   ##
========================================
  Coverage    79.37%   79.37%           
========================================
  Files          166      166           
  Lines        13878    13878           
  Branches       705      705           
========================================
  Hits         11016    11016           
  Misses        2710     2710           
  Partials       152      152           
Flag Coverage Δ
e2e 85.18% <ø> (ø)
unit 43.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@sidharthv96
Copy link
Member

What we normally do to support unicode is quoted strings, like below.

["] { this.begin("string"); }
<string>["] { this.popState(); }
<string>[^"]* { return "txt"; }
"pie" return 'PIE';

I'm not sure of the implications of the suggested approach of adding the unicode character range. @nirname @aloisklink any potential issues?

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After having a look at the JISON file for ER Diagrams, I believe every field that uses 'ALPHANUM' already supports 'ENTITY_NAME' (which is confusingly named), which does support unicode characters, as long as they're quoted, e.g.

```mermaid
erDiagram
    "🌋" ||--o{ "Hiking Trail 🥾" : "contains 🧭"
    "🌋" {
        string name "name: 名"
        float height "my 📈"
        string sector
    }
```

renders as

erDiagram
    "🌋" ||--o{ "Hiking Trail 🥾" : "contains 🧭"
    "🌋" {
        string name "name: 名"
        float height "my 📈"
        string sector
    }
Loading

Therefore, this change isn't needed. However, the documentation for this is missing, if you want to add it @Mikek16.

Both attribute types and attribute names don't support unicode.

It would be nice if we could somehow also throw a nicer error message when somebody tries to use unicode characters and they're not using the "" syntax, but I don't know if jison has support for that.

@nirname
Copy link
Contributor

nirname commented Jun 20, 2024

ENTITY_NAME should be enough, but I don't have any objections about extending ALPHANUM in this case. We have to ensure that EOF is not in that unicode range.

But as @aloisklink suggested, I would rather have documentation updated first, and the see how many people still encounter this problem

@nirname
Copy link
Contributor

nirname commented Jun 20, 2024

@Mikek16 gentle remidner, just want to know if there is any further work expected on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
Status: Opinion
Development

Successfully merging this pull request may close these issues.

Entity Relation Chart do not support non-ascii character
4 participants