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

Adds parser support for commas in ER diagram attribute types #5128

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

Conversation

jerluc
Copy link

@jerluc jerluc commented Dec 8, 2023

📑 Summary

This short patch adds support for commas in the attribute types of ER diagrams. This helps to support commonly used SQL types such as NUMERIC(precision,scale) (or DECIMAL) or GEOMETRY(geometry_type,SRID). While ER diagrams are not exclusive to SQL, this kind of additional support may help others to leverage Mermaid's ER diagrams for the use case of describing SQL databases, without impacting functionality for those who aren't using ER diagrams for SQL.

AFAIK, this has not yet been reported as an issue.

📏 Design Decisions

The implementation is pretty straightforward:

  • Added a , to the set of accepted characters in the ER diagram grammar (maybe this needs to be even more sophisticated by looking specifically for a comma-delimited list of tokens only within parentheses? let me know what you think!)
  • Added a unit test to the ER diagram parser spec to ensure that it works

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added necessary unit/e2e tests.
  • 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features. I did not add any new documentation, as this might be considered a bug vs. a new feature. Let me know if you think this is relevant for this patch!
  • 🔖 targeted develop branch

Copy link

netlify bot commented Dec 8, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 29bc84f
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/661d921662127100082a7531
😎 Deploy Preview https://deploy-preview-5128--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.

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 5.74%. Comparing base (0d8fe3b) to head (29bc84f).
Report is 1513 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5128   +/-   ##
=======================================
  Coverage     5.74%   5.74%           
=======================================
  Files          276     276           
  Lines        41905   41905           
  Branches       514     514           
=======================================
  Hits          2407    2407           
  Misses       39498   39498           
Flag Coverage Δ
unit 5.74% <ø> (ø)

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

@sidharthv96 sidharthv96 requested a review from nirname January 29, 2024 08:35
@SleepWalker
Copy link

Really great and important feature. It's only one character diff. Would be great if review won't take ages 🙏

@jerluc
Copy link
Author

jerluc commented Mar 9, 2024

@nirname @sidharthv96 anything I can do to help get this merged?

@chrisafl
Copy link

+1 this functionality is needed to generate accurate ERD data types.

@nirname nirname requested a review from sidharthv96 May 29, 2024 22:24
@nirname
Copy link
Contributor

nirname commented May 29, 2024

Hello! Thanks for the contribution. There are another PR with slightly different approach, which presumably covers the same subject

The latter one is slightly newer than this, and some documentation are still missing, but it would be great to solve the issue one way or another. I think that escapement is more diverse and extensible

Copy link
Contributor

@nirname nirname left a comment

Choose a reason for hiding this comment

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

Code is OK, but I would prefer another PR which covers this subject also, but not limited to

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.

4 participants