Skip to content

[CLEANUP] Order class names in the class diagram alphabetically #1299

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
Jun 30, 2025

Conversation

oliverklee
Copy link
Collaborator

No description provided.

@oliverklee oliverklee requested review from sabberworm and JakeQZ June 29, 2025 15:06
@oliverklee oliverklee self-assigned this Jun 29, 2025
@oliverklee oliverklee added documentation cleanup developer-specific Issues that only affect maintainers, contributors, and people submitting PRs labels Jun 29, 2025
@oliverklee oliverklee force-pushed the cleanup/reorder-classes branch from 819128d to 601cb8b Compare June 29, 2025 15:06
@coveralls
Copy link

coveralls commented Jun 29, 2025

Coverage Status

coverage: 57.935%. remained the same
when pulling 31866ca on cleanup/reorder-classes
into 766238d on main.

Copy link
Collaborator

@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'm not entirely sure of the rationale behind the reordering, but since there was no specific order prevoiusly (other than to try and get the class diagram to render more clearly, which didn't work), it's fine.

However, I think lexicographical ordering would be better than ASCII, and the blank line after the config settings should be kept.

@@ -621,109 +621,108 @@ class Sabberworm\CSS\CSSList\Document#4 (2) {
```mermaid
classDiagram
direction LR

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer the blank line between the settings and the content to be retained.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment on lines +651 to +654
class CSSString {
}
class CalcFunction {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my alphabet, 'a' comes before 's'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The diagram generator we're using seems to sort an uppercase "S" before a lowercase "a", it seems.

@oliverklee oliverklee force-pushed the cleanup/reorder-classes branch from 601cb8b to 4761d46 Compare June 30, 2025 13:52
@oliverklee
Copy link
Collaborator Author

I'm not entirely sure of the rationale behind the reordering

The rationale is to make the diff smaller in the PR that uses the updated autogenerated class diagram.

@oliverklee oliverklee requested a review from JakeQZ June 30, 2025 13:57
Copy link
Collaborator

@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.

The rationale is to make the diff smaller in the PR that uses the updated autogenerated class diagram.

The diagram generator we're using seems to sort an uppercase "S" before a lowercase "a", it seems.

I understand now. Thanks.

@JakeQZ JakeQZ merged commit 1814dd7 into main Jun 30, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/reorder-classes branch June 30, 2025 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup developer-specific Issues that only affect maintainers, contributors, and people submitting PRs documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants