Enteee / plantuml-parser

Parse PlantUML with JavaScript or TypeScript
https://duckpond.ch/category/plantuml-parser
Apache License 2.0
136 stars 33 forks source link

C4 PlantUML elements with parameters passed with $ are not recognized #100

Open philippsimon opened 10 months ago

philippsimon commented 10 months ago

Describe the bug

When I add elements and pass the arguments like it's shown on the C4 PlantUML page with a $-character ($descr="description") these elements are not recognized.

In general nearly all examples on the C4 PlantUML page can't be displayed because of that issue.

To Reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

PlantUML

@startuml
Person(person_descr, "person", $descr="description")
Person(person_descr, "person", $descr="") ' an empty parameter also doesn't work
Person(person_tags_one, "person", $tags="one+two")
@enduml

Here more and it shows that it's a valid syntax

Expected behavior The elements should be returned.

Enteee commented 10 months ago

@dgassma : since all the c4 stuff is coming from you, do you have an idea why this is happening?

dgassma commented 10 months ago

@philippsimon : you are right and I am sorry for the inconvenience. I misinterpreted the meaning of the $.

The current implementation is partly optional parameter parsing. Take a look at the C4 tests in plantuml-parser:

@startuml

("C4 - Person & System")

Person(person, "Person::label", "Person::descr")
Person_Ext (person_ext, "Person_Ext::label", "Person_Ext::descr")

System(system, "System::label", "System::descr")
System_Ext (system_ext, "System_Ext::label", "System_Ext::descr")

SystemDb (systemDb, "SystemDb::label", "SystemDb::descr")
SystemDb_Ext (systemDb_ext, "SystemDb_Ext::label", "SystemDb_Ext::descr")

SystemQueue (systemQueue, "SystemQueue::label", "SystemQueue::descr")
SystemQueue_Ext (systemQueue_ext, "SystemQueue_Ext::label", "SystemQueue_Ext::descr")

System (system, "System::label")

System (systemDescr, "System::label", "System::descr")

System (systemDescrSprite, "System::label", "System::descr", "System::sprite")

System (systemDescrSpriteTags, "System::label", "System::descr", "System::sprite", "System::tags")

System (systemDescrSpriteTagsLink, "System::label", "System::descr", "System::sprite", "System::tags", "System::link")

@enduml

I already have an idea how to fix it in my mind. However, I am not sure, if the library should support the old (wrong) parsing as well? What is your opinion on that @philippsimon & @Enteee

I am going to try to provide a fix by the end of week.

philippsimon commented 10 months ago

@dgassma I would actually allow both. I tested it and both works. So I guess others could have interpreted the same as you and it also works. I tested it here: https://www.plantuml.com/plantuml/uml/TOzF2u8m68VlVWgJ3WNf83e9WM3T2eBeBFlpaeDjolQExDitmmaHtiRps_F0Mpca5hrMoKeQhho0j4NiNKc8fQ_YAR7rp3kml3C81WlUQT8hQaOdToYKPAG-vBSHFQuNegb3i04rTJ2eD62BVkmdztTOz8bjtFJOfLbaPQae0vNz806EssmxYyrvm5bkOC3uUzfYv0_7q79flvXQV-NZKh428y9r7m00

philippsimon commented 10 months ago

I just saw that for mermaidjs they already created a parser for C4 diagrams: https://github.com/mermaid-js/mermaid/blob/develop/packages/mermaid/src/diagrams/c4/parser/c4Diagram.jison Maybe it can be reused or a collaboration can be started?

Enteee commented 9 months ago

@dgassma i don't have a strong opinion if we should support the old parsing as well. I rarely use C4 so I can not really speak from experience. Sorry..

@philippsimon looking at the parsing grammar in mermaid; looks cool and I would be super happy if I don't have to support C4 in a PEG in this project. But I am not quite sure if there's a working Jison to PEG translator and I am not even quite sure if our grammar isn't better, i think @dgassma did quite a good job :)