-
Notifications
You must be signed in to change notification settings - Fork 145
[DOCS] Regenerate the class diagram in the README #1298
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
base: main
Are you sure you want to change the base?
Conversation
Marking this as draft for now. I'll first reorder the classes in the diagram to make this PR here a bit smaller. |
It would be nice if the diagram had fewer criss-crossing paths. AI should be good at that, but seems to be vastly overrated :/ |
dff73f2
to
c2891db
Compare
c2891db
to
207d21e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous source for the diagram had entries like
URL --> "1" CSSString: url
whereas now we have
URL ..> CSSString: dependency
So we no longer identify the property that has the dependency (or whether it's as a single object, array, or whatever). Is there any reason for this change?
The old version of the generator which we used did not recognize associations at all. That's why I manually added those to the diagram, including the name of the corresponding property. The current version of the generator does recognize associations, even though it only labels them as "dependency", but not with the name of the corresponding property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a bit of time reordering the original 'relationships' section, after changing to the now-generated syntax, to conclude that there a quite a few differences there.
I think this needs three separate pre-patches so the diffs can be more clearly seen (along the lines of #1299):
- Move
class URL
to be ASCII-sorted; - Change to use
..>
,dependency
and remove"*"
/"1"
; - Reorder the relationships to be consistent with the generated order.
For reference, what I did for reviewing is on the branch at https://github.com/MyIntervals/PHP-CSS-Parser/tree/docs/class-diagram-review and includes all three of the above, which need to be split. And I may have made mistakes: there was a lot of cutting and pasting
Makes sense, will do. |
d98dc23
to
adeb8fb
Compare
This is in preparation for #1298.
This is in preparation for #1298.
This is in preparation for #1298.
This is in preparation for #1298.
8632f7d
to
fe66e2d
Compare
This is in preparation for #1298.
This is in preparation for #1298.
fe66e2d
to
be15675
Compare
This is in preparation for #1298.
This is in preparation for #1298.
be15675
to
a775112
Compare
This is in preparation for #1298.
This is in preparation for #1298.
a775112
to
54eef10
Compare
This is in preparation for #1298.
This is in preparation for #1298.
54eef10
to
052484e
Compare
This is in preparation for #1298.
This is in preparation for #1298.
The class diagram is (almost) unchanged to the output from `tasuku43/mermaid-class-diagram`, except for the kept `direction LR`. This is in preparation for adding a script for auto-creating the class diagram in #1297.
052484e
to
0632a8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it's adding all dependencies, even if they are only passed as method parameters, and not actually stored in class properties. This rather clutters the class diagram. The current one is reasonably easy to understand. The diagram after this change harder to understand and about three times the size.
Would it be possible to exclude dependencies that are only ever used as method parameters?
There seems to be a bug in that traits are completely ignored.
AtRule <|.. AtRuleSet: realization | ||
AtRuleSet ..> OutputFormat: dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's now also picking up dependencies that are method parameters, not just properties.
Value <|-- ValueList: inheritance | ||
|
||
CSSList ..> CSSList: dependency | ||
CSSList ..> Comment: dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's losing dependencies that are class properties via traits. The CommentContainer
trait was added for classes that have Comment
s. Given that traits are not included in the class diagram, this looks to me like a bug in the generator (for which I think the better fix would be to include traits in their own right, like interfaces and classes).
OutputFormat ..> OutputFormat: dependency | ||
Rule ..> Comment: dependency | ||
RuleSet ..> Comment: dependency | ||
ValueList ..> Value: dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValueList
extents Value
and also has Value
s as properties. It looks like the latter connection is being lost.
Okay, putting this on the backburner until the diagram generator has these two bugfixes/features. I've created two tickets for that: |
The class diagram is (almost) unchanged to the output from
tasuku43/mermaid-class-diagram
, except for the keptdirection LR
.This is in preparation for adding a script for auto-creating the class diagram in #1297.