Skip to content

[DOCS] Add a class diagram #482

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

Merged
merged 2 commits into from
Feb 26, 2024
Merged

[DOCS] Add a class diagram #482

merged 2 commits into from
Feb 26, 2024

Conversation

oliverklee
Copy link
Contributor

@oliverklee oliverklee commented Feb 17, 2024

This diagram aims to provide an overview of which classes we have and how their are interconnected. Hence, it does not contain any methods or properties.

As the Mermaid render on GitHub does not support namespace, the
classes are not grouped into namespaces in the diagram.

Fixes #422

@oliverklee oliverklee self-assigned this Feb 17, 2024
@oliverklee oliverklee marked this pull request as draft February 17, 2024 14:18
@oliverklee oliverklee force-pushed the docs/class-diagram branch 9 times, most recently from 7ffbb84 to d6e9c21 Compare February 24, 2024 11:31
@oliverklee oliverklee marked this pull request as ready for review February 24, 2024 11:31
@oliverklee
Copy link
Contributor Author

The rendered class diagram can be viewed at https://github.com/MyIntervals/PHP-CSS-Parser/tree/docs/class-diagram .

This diagram aims to provide an overview of which classes we have
and how their are interconnected. Hence, it does not contain
any methods or properties.

As the Mermaid render on GitHub does not support namespace, the
classes are not grouped into namespaces in the diagram.

Fixes #422
@sabberworm
Copy link
Contributor

@oliverklee did you generate this diagram manually or was it auto-generated by some tool?

@oliverklee
Copy link
Contributor Author

I created it manually (by reading the sourcecode).

Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created it manually (by reading the sourcecode).

A good excercise to get an overview and understanding of the codebase 👍. However, this seems to me like something an automated tool should do well. Is there not something that can auto-generate the Mermaid mark-up? It would make updating easier following changes.

I've only looked at a small section of the diagram so far; I've marked up some comments:

class-diagram-review

If you want to quote-reply, here they are:

  • This copies the code rather than the diagram. Is there a way to get the diagram as a PNG for easier viewing, or perhaps printing out?
  • This should be over here. Do we have any control over the layout?
  • aComponents is not used by RuleValueList, though is used by CalcRuleValueList. Should the arrow bypass (or be chained)?

This should be over here. Do we have any control over the layout?

I saw there was a left-to-right setting, but it doesn't seem to be working properly. The rendered diagram comes out as a jumbled-up mess, when it could obviously be rendered much more clearly by enforcing that inheritees are always to the left of their ancestors, with a wider peice of paper provided if necessary.

@oliverklee
Copy link
Contributor Author

oliverklee commented Feb 25, 2024

However, this seems to me like something an automated tool should do well. Is there not something that can auto-generate the Mermaid mark-up?

I've done some googling and packagist-ing, and I'm going to have a look at https://github.com/tasuku43/php-mermaid-class-diagram.

Is there a way to get the diagram as a PNG for easier viewing, or perhaps printing out?

On GitHub, you can zoom in with the
grafik
button. Also, there's a PhpStorm plugin called "mermaid" which I use locally. Other than than, I don't know.

This should be over here. Do we have any control over the layout?

No, I don't think so. (Reordering things might have an effect, but I'd prefer keeping the thing in the Mermaid code in the same order as in the PHP code.)

aComponents is not used by RuleValueList, though is used by CalcRuleValueList. Should the arrow bypass (or be chained)?

The associations work the other way around: ValueList has an association aComponents to RuleValueList|CSSFunction|CSSString|LineName|Size|URL.

I saw there was a left-to-right setting, but it doesn't seem to be working properly.

I've fiddled around with it a bit as the default setting created a diagram that was horizontally very wide, which caused it to get tiny within the rendered Markdown file. LR and RL cause the resulting diagram to be less wide, while TB and BT (one of which is the default, I'd guess) make it wider.

@oliverklee
Copy link
Contributor Author

oliverklee commented Feb 25, 2024

I'm going to have a look at https://github.com/tasuku43/php-mermaid-class-diagram.

I've given it a go. These are the results:

  • We'll need to install this package in our project for it to be able to analyze our code base. Because of dependency hell, we can only install this package if we drop the codacy/coverage dependency. (I'll try out something else in a minute.)
  • The generated diagram will be in a separate file without the "Mermaid in Markdown" marker. That's something that we can work around by copy'n'pasting, I think.
  • Most important: The generated diagram does not include the associations between classes, but only inheritance/implementation.

@oliverklee
Copy link
Contributor Author

The generated diagram will be in a separate file without the "Mermaid in Markdown" marker.

According to the tool docs, there is an argument for the format, but the tool refuses to accept it.

We'll need to install this package in our project for it to be able to analyze our code base.

No, we don't. I've tried to have the tool in a separate repository and analyze our code, and that also works.

(dependency hell)

I've checked https://phar.io/, and this tool is not available via PHAR.

The generated diagram does not include the associations between classes, but only inheritance/implementation.

Maybe we can automate this in some way that we only need to manually add the associations and have the other things generated. What do you think?

@oliverklee
Copy link
Contributor Author

I’d like to use the autogenerated diagram in the future and see two possible approaches here:

a) merge this PR and then create a dedicated PR to switch to the auto-generated class diagram (plus hand-crafted associations)
b) adapt this PR to include the auto-generated class diagram

Both approaches should include some documentation about updating the generated part of the class diagram.

I’d prefer approach a) so we can go in smaller steps and have the first version of the class diagram merged soon.

@JakeQZ @sabberworm What do you think?

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 25, 2024

I’d prefer approach a) so we can go in smaller steps and have the first version of the class diagram merged soon.

I agree. Let's get it in first, then refine it.

@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 26, 2024

I've fiddled around with it a bit as the default setting created a diagram that was horizontally very wide, which caused it to get tiny within the rendered Markdown file. LR and RL cause the resulting diagram to be less wide, while TB and BT (one of which is the default, I'd guess) make it wider.

I'd rather have the rendered diagram always show descendent classes to the left of the ancestor. What I pointed out seems to be a bug in the rendering. I am not bothered about it being wider - it can't be easily viewed within GitHub's markdown viewer anyway (the zooming and scrolling tools aren't the best), which is why I asked if it could be rendered as a PNG (actually, ideally, PDF or SVG).

The associations work the other way around: ValueList has an association aComponents to RuleValueList|CSSFunction|CSSString|LineName|Size|URL.

Ah. I see. There is a slight design flaw here. A parent class should never be referencing a child by name. #499 should solve that.

`` ` joke

My sister just gave birth to non-identical twins: a girl and a boy. She was too exhausted to fill in the forms, and hadn't really decided on names anyway, so, in a siblingly gesture, allowed me to name them.

For the girl, I chose Denise. For the boy, I chose Denephew.

`` `

@oliverklee oliverklee merged commit 8f94950 into main Feb 26, 2024
@oliverklee oliverklee deleted the docs/class-diagram branch February 26, 2024 13:44
@JakeQZ
Copy link
Contributor

JakeQZ commented Feb 27, 2024

There is a slight design flaw here. A parent class should never be referencing a child by name. #499 should solve that.

I'm going to disagree with myself a bit now. The parent might create objects of a child type (e.g. in static Value::parseValue()), but should not have methods or properties with type specifications involving a child type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a class diagram to the README
3 participants