jfeliu007 / goplantuml

PlantUML Class Diagram Generator for golang projects
MIT License
1.84k stars 172 forks source link

Struct Fields are not part of the diagram #34

Closed fabiante closed 4 years ago

fabiante commented 4 years ago

Hi there!

First of all, nice work, like the idea of this repo!

I noticed, that this code looks like this:

image

But I would have expected something like this:

image

which would correspond to this PlantUML:

@startuml
namespace plantuml {
    class Phone << (S,Aquamarine) >> {
        + Value string
        + IsMobile bool

    }
    class Person << (S,Aquamarine) >> {
        + Birthday Birthday
        + Phones []Phone

    }
    class Birthday << (S,Aquamarine) >> {
        + Time time.Time

    }
    Phone <|-- Person
    Birthday <|-- Person
}
@enduml

I would like to use this tool to create a quick ERD for an upcoming project. And doing this by code enables me to quickly generate these models.

My proposal:

If displaying links based on struct fields is not considered a standard, could this at least be added as an CLI option?

jfeliu007 commented 4 years ago

Hi @FabianTe , Thanks for the comment.

It looks like maybe aggregation could be the solution for this. Since a person Has-A phone and Has-A Birthday.

Possibly resulting on something like this

@startuml
namespace plantuml {
    class Phone << (S,Aquamarine) >> {
        + Value string
        + IsMobile bool

    }
    class Person << (S,Aquamarine) >> {
        + Birthday Birthday
        + Phones []Phone

    }
    class Birthday << (S,Aquamarine) >> {
        + Time time.Time

    }
    Person o-- Phone
    Person o-- Birthday
}
@enduml

aggregation

Is this what you are looking for with this issue? I think I thought of adding this before, but it might have been too clogged for the diagram. If this specific configuration suits you I can definitely consider it as part of the Parser and we can include an option on the CLI.

If what you are looking for is changing the definition of the <|-- link to mean what you described, then I guess this should entail a longer discussion, since that could alter the meaning of the diagram.

fabiante commented 4 years ago

Is this what you are looking for with this issue? I think I thought of adding this before, but it might have been too clogged for the diagram. If this specific configuration suits you I can definitely consider it as part of the Parser and we can include an option on the CLI.

Yes

jfeliu007 commented 4 years ago

Ok, I'll work on this one.

jfeliu007 commented 4 years ago

Hi @FabianTe I made an update to master with this implemented. I will welcome any comments regarding this. Thanks for your suggestion. I will make this part of the next release. But for now, it will be on the master branch only.

I am thinking of cutting a release in the following days. Best regards.

fabiante commented 4 years ago

Thank you! That was really quick, nice.

jfeliu007 commented 4 years ago

No problems. Thanks for your suggestion. Keep in mind I initially set a flag -aggregation that I quickly deprecated in favor of a different suggestion.

These are considered aggregations so take a look at the Readme and specifically the usage section, you will see the flag in there -show-aggregations. Let me know if this works for you 👍

jfeliu007 commented 4 years ago

By the way. This update will only show aggregations for Public fields. I think if we include every field the diagram would be unreadable.