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

Problems with composition feature #71

Open
fuhrmanator opened this issue Jul 12, 2021 · 4 comments
Open

Problems with composition feature #71

fuhrmanator opened this issue Jul 12, 2021 · 4 comments

Comments

@fuhrmanator
Copy link
Contributor

fuhrmanator commented Jul 12, 2021

First, thanks for making tplant! It is very useful!

The composition flag is good, but I'm suggesting the following improvements. Let me use this example from test/Raytracer/Scene.ts:

export interface Scene {
    things: Thing[];
    lights: Light[];
    camera: Camera;
}

The PlantUML produced using the composition flag should be

Scene --> "things" Thing
Scene --> "lights" Light
Scene --> "camera" Camera

Here are my reasons:

  • it should default to a simple association, because it's wrongly showing all associations as whole/part (composition, i.e., *--), many of which are not truly the semantics of composition. It's pretty hard to figure that out from static analysis of source code, I think. Edit: Furthermore, aggregation (to many) isn't really any different than an association in UML, so a simpler link is just to use --.
  • it could/should include navigability, since you can see it from the TypeScript code, e.g. Scene.things: Thing[]; implies one can navigate from the class Scene to one or more Thing. Edit: Hence, I suggest -->.
  • it could contain cardinalities on the association. However, PlantUML has trouble to show both the names of the ends, e.g., "things" Thing as above, and a cardinality such as "*" Thing. Maybe combining them?
Scene --> "things\n*" Thing

which doesn't always look so great, but is useful I think.

Edit: in cases of a single reference, e.g. myThing: Thing the cardinality would be "1"

I'd be happy to try a PR to support some of these things (with tests, of course), if you think they're reasonable.

@bafolts
Copy link
Owner

bafolts commented Jul 12, 2021

Putting up a PR sounds great.

@fuhrmanator
Copy link
Contributor Author

SInce you merged, can you publish the package on npm?

@bafolts
Copy link
Owner

bafolts commented Aug 6, 2021

Published.

@loopingz
Copy link
Contributor

The issue should probably be closed now, I noticed the 2.3.4, 2.3.5, 2.3.6 and 3.0.0 tags / releases are missing from github

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

No branches or pull requests

3 participants