Skip to content

[TASK] Mark OutputFormatter as @internal #896

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.

Sign up for GitHub

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

Jump to bottom
Merged
oliverklee merged 1 commit into from
Mar 4, 2025
Merged

[TASK] Mark OutputFormatter as @internal #896

oliverklee merged 1 commit into from
Mar 4, 2025

Conversation

@oliverklee
Copy link
Collaborator

@oliverklee oliverklee commented Feb 10, 2025

This class should only be used for formatting CSS from within this library. It is not intended to be called from outside.

@oliverklee oliverklee self-assigned this Feb 10, 2025
@oliverklee oliverklee force-pushed the task/internal-formatter branch from 79d9544 to ec45f29 Compare February 10, 2025 08:51
@coveralls
Copy link

coveralls commented Feb 10, 2025 •
edited
Loading

Coverage Status

coverage: 55.726%. remained the same
when pulling 900883c on task/internal-formatter
into 8dcc1a5 on main.

@oliverklee oliverklee mentioned this pull request Feb 10, 2025
Merged
@oliverklee oliverklee force-pushed the task/internal-formatter branch 2 times, most recently from 20bb6df to f20a6af Compare February 10, 2025 10:57
JakeQZ
JakeQZ reviewed Feb 11, 2025
View reviewed changes
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 think so.

@sabberworm, would you agree?

@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 11, 2025

Actually maybe not. See #895 (comment).

It might not be used from the outside, but may need to be constructed and passed around, depending on how we resolve the circular dependency.

@oliverklee oliverklee force-pushed the task/internal-formatter branch from f20a6af to 1696d58 Compare February 11, 2025 11:09
@oliverklee
Copy link
Collaborator Author

oliverklee commented Feb 11, 2025

I've proposed a path forward in #907.

@oliverklee oliverklee force-pushed the task/internal-formatter branch 3 times, most recently from 370dee5 to 8c36d23 Compare February 14, 2025 11:23
@oliverklee
Copy link
Collaborator Author

oliverklee commented Feb 14, 2025

@sabberworm ping

@oliverklee oliverklee force-pushed the task/internal-formatter branch from 8c36d23 to 0a17c5c Compare February 14, 2025 22:30
@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 15, 2025

Marking as draft pending #907,

@JakeQZ JakeQZ marked this pull request as draft February 15, 2025 02:03
@oliverklee oliverklee force-pushed the task/internal-formatter branch 9 times, most recently from 075e5d3 to c7b73db Compare February 20, 2025 10:16
@oliverklee oliverklee force-pushed the task/internal-formatter branch 8 times, most recently from 542332d to e3bf18c Compare February 27, 2025 11:21
@oliverklee oliverklee force-pushed the task/internal-formatter branch 10 times, most recently from efa0105 to fe78fb7 Compare March 4, 2025 08:16
@oliverklee oliverklee marked this pull request as ready for review March 4, 2025 08:17
@oliverklee oliverklee force-pushed the task/internal-formatter branch from fe78fb7 to 7eb5934 Compare March 4, 2025 18:05
@oliverklee
Copy link
Collaborator Author

oliverklee commented Mar 4, 2025

Ping @JakeQZ @sabberworm - I'd like to get this merged soon-ish so I can reduce the number of open PRs created by me (which reduces the amount of rebasing I need to do).

@oliverklee
[TASK] Mark OutputFormatter as @internal
900883c 
This class should only be used for formatting CSS from
within this library. It is not intended to be called
from outside.
@oliverklee oliverklee force-pushed the task/internal-formatter branch from 7eb5934 to 900883c Compare March 4, 2025 18:23
@JakeQZ
Copy link
Collaborator

JakeQZ commented Mar 4, 2025

Yes, I think currently users of this library provide an OutputFormat method which internally has a reference to an OutputFormatter which does the work and maintains state. I guess it's analogous to a Car and an Engine. The car has an engine rather than the other way round, and the user drives the car without needing to touch the engine directly. The same car can visit the various CSS elements.

Also, this is consistent with the proposed change in #898. So 👍 from me now.

@oliverklee oliverklee merged commit 5421a6d into main Mar 4, 2025
21 checks passed
@oliverklee oliverklee deleted the task/internal-formatter branch March 4, 2025 18:27
oliverklee added a commit that referenced this pull request Mar 4, 2025
@oliverklee
[TASK] Mark OutputFormatter as @internal
afd6349 
This is the V8.x backport of #896.
@oliverklee oliverklee mentioned this pull request Mar 4, 2025
Merged
JakeQZ pushed a commit that referenced this pull request Mar 4, 2025
@oliverklee
[TASK] Mark OutputFormatter as @internal (#1073)
9c48d3a 
This is the V8.x backport of #896.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JakeQZ JakeQZ JakeQZ approved these changes

@sabberworm sabberworm Awaiting requested review from sabberworm

Assignees

@oliverklee oliverklee

Labels

to-backport

Projects

None yet

Milestone

9.0.0: Drop support for PHP < 7.2 and...

Development

Successfully merging this pull request may close these issues.

4 participants

@oliverklee @coveralls @JakeQZ