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

fix: mermaid complex types #119

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Conversation

cobac
Copy link
Contributor

@cobac cobac commented Jul 15, 2024

Previously, from a complex nested type like a<b<c d, e f>>, the mermaid adapter would ignore a and capture b, c d and , e f.
Now it captures a and b<c d, e f>, which is discarded as far as I can see.

I've simplified the regex expression and tested with the obvious cases, but I'm not sure if the new regex might miss some edge cases. It'd be nice to add more tests if you think I might be missing something :)

I'm pushing twice, without the fix implemented and then with the fix, so that you can see the testing logs in both cases. well workflow needs approval anyway 😅

Thanks for your work!

Checklist

  • I have read the contributing guide and understand what's expected of me
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR

Simplifying the regex expression.

Previously from `a<b<c d, e f>>` it would ignore `a` and capture `b`, `c d` and `, e f`.
Now it captures `a` and `b<c d, e f>`, which is disregarded as far as
I can see.
@datnguye
Copy link
Owner

Hi @cobac thanks for promptly fixing this!

Let me review and get back to you! But first sign, I think it does make sense with the changes when looking into the tests.
By the way, @syou6162 do you mind to collab with this PR in the meantime.

Thanks

@datnguye
Copy link
Owner

With the previous regex: (\w+)<(\w+\s+\w+(\s*,\s*\w+\s+\w+)*)>

image

Current proposal: (\w+)<.*>

image

Given that result, I think the previous regex is done to a specific type struct only, and this proposal is somewhat better if we try to stick to that "only get the root type".

Currently, I'm wondering if we can try to do more as follows:

  • ARRAY<NotStruct> --> ARRAY_NotStruct
  • ARRAY<STRUCT<...>> --> ARRAY_STRUCT[OMITTED]

Let me know what you think?
If not, and @syou6162 won't disagree by tmr, I shall release this to v1.15

Thanks

@cobac
Copy link
Contributor Author

cobac commented Jul 15, 2024

Thanks for the quick reply!

I have no particular opinions about how to represent the complex type in the diagram.

A release soon would be ideal since I'm trying to put this in production 😊 .

@datnguye datnguye merged commit 1855328 into datnguye:main Jul 17, 2024
10 checks passed
@datnguye
Copy link
Owner

v1.15.0 is now GA 🥳

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.

2 participants