search

anz-bank / sysl

Sysl (pronounced "sizzle") is a system specification language
https://sysl.io
Apache License 2.0
122 stars 42 forks source link

Refactorforcatalog #866

Closed ashwinsajiv closed 4 years ago

ashwinsajiv commented 4 years ago

Data model diagram for mermaid for a specific type

codecov[bot] commented 4 years ago

Codecov Report

Merging #866 into master will decrease coverage by 0.31%. The diff coverage is 74.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #866      +/-   ##
==========================================
- Coverage   83.66%   83.35%   -0.32%     
==========================================
  Files          72       73       +1     
  Lines       10458    10609     +151     
==========================================
+ Hits         8750     8843      +93     
- Misses       1375     1432      +57     
- Partials      333      334       +1
Impacted Files Coverage Δ
pkg/mod/fs.go 69.81% <52.94%> (ø)
pkg/parse/listener_impl.go 89.50% <66.66%> (ø)
pkg/mermaid/datamodeldiagram/datamodeldiagram.go 58.57% <73.43%> (ø)
...endpointanalysisdiagram/endpointanalysisdiagram.go 82.97% <100.00%> (ø)
...g/mermaid/integrationdiagram/integrationdiagram.go 84.37% <100.00%> (+3.24%) :arrow_up:
pkg/mermaid/sequencediagram/sequencediagram.go 74.19% <100.00%> (ø)
pkg/parse/parse.go 83.97% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 66e0c7e...b23ddb6. Read the comment docs.

ashwinsajiv commented 4 years ago

Looks really good overall, you've split up the functions really well so they're nice and small. Just a few small nitpicks about duplication of strings between datamodeldiagram.go and datamodeldiagramwithappandtype.go

Also, have you added some new tests for datamodeldiagramwithappandtype.go?

One last nitpick: Could we possibly use something a bit shorter than datamodeldiagramwithappandtype.go?

I actually wouldn't mind seeing datamodeldiagramwithappandtype moved into datamodeldiagram.go since it's handling the same concern: generating datamodel diagrams. If you needed to change some behaviour in the future, you'd want to make changes to both files. Also that way, it's easier to see any duplications, and you can put for example GenerateDataModelDiagramWithAppAndType right below GenerateDataModelDiagram. Thoughts?

Yeah can do that, there aren't going to be any duplication but the feature itself was kinda getting bigger and I wanted to split it. I'll put them together if that is more readable.