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

3952 - lexical ids #4785

Closed
wants to merge 12 commits into from
Closed

3952 - lexical ids #4785

wants to merge 12 commits into from

Conversation

jgreywolf
Copy link
Contributor

@jgreywolf jgreywolf commented Aug 28, 2023

📑 Summary

Changed ids of edge paths for relationships to use class names for easier identification in code

This includes the changes in #4582 and #4534, so those should be merged first

Resolves #3952
image

@netlify
Copy link

netlify bot commented Aug 28, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 26454a4
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/650b81a4202b3d0008c18ecc
😎 Deploy Preview https://deploy-preview-4785--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.

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #4785 (f522627) into develop (8be8736) will decrease coverage by 5.94%.
The diff coverage is 90.32%.

❗ Current head f522627 differs from pull request most recent head 26454a4. Consider uploading reports for the commit 26454a4 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4785      +/-   ##
===========================================
- Coverage    80.50%   74.57%   -5.94%     
===========================================
  Files          148      146       -2     
  Lines        13024    14529    +1505     
  Branches       612      613       +1     
===========================================
+ Hits         10485    10835     +350     
- Misses        2405     3569    +1164     
+ Partials       134      125       -9     
Flag Coverage Δ
e2e 80.67% <90.32%> (-5.07%) ⬇️
unit 45.25% <ø> (+1.57%) ⬆️

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

Files Changed Coverage Δ
packages/mermaid/src/diagrams/class/classDb.ts 82.35% <ø> (-0.45%) ⬇️
packages/mermaid/src/dagre-wrapper/edges.js 77.56% <50.00%> (-6.41%) ⬇️
packages/mermaid/src/dagre-wrapper/markers.js 98.21% <96.00%> (-1.79%) ⬇️
...ges/mermaid/src/diagrams/class/classRenderer-v2.ts 80.14% <100.00%> (+0.29%) ⬆️

... and 67 files with indirect coverage changes

@sidharthv96 sidharthv96 marked this pull request as draft August 28, 2023 13:36
@sidharthv96
Copy link
Member

Drafting till #4582 and #4534 are merged.

@jgreywolf jgreywolf marked this pull request as ready for review September 5, 2023 16:01
id: 'id' + cnt,
id: 'id_' + edge.id1 + '_' + edge.id2,
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only relevant part for lexical IDs?
If so, can you reset your branch to develop and only have this in the PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think the cnt should also be in the ID.

Otherwise, id of [a, b_c] and [a_b, c] will both be id_a_b_c, leading to a conflict.

@jgreywolf
Copy link
Contributor Author

will resubmit

@jgreywolf jgreywolf closed this Sep 20, 2023
@jgreywolf jgreywolf deleted the 3952-lexical-ids branch November 8, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class Diagram edgePaths should have lexical ids
3 participants