[FEATURE] Add dedicated property accessors for `OutputFormat` #871

oliverklee · 2025-02-02T17:48:17Z

No description provided.

coveralls · 2025-02-02T17:49:55Z

coverage: 49.156% (+3.5%) from 45.629%
when pulling ec1aa5a on feature/property-accessors
into 7a12457 on main.

JakeQZ

Hot on the heels of #880. Looks good, but

Some parameter names seem to be inaccurate;
The test of the deprecated array|string functionality shouldn't be changed;
I'm wondering if any or all of the private properties should or shouldn't have accessors (but am unsure).

JakeQZ · 2025-02-06T18:15:22Z

src/OutputFormat.php

+    public function setBeforeAtRuleBlock(string $whitespace): self
+    {
+        $this->sBeforeAtRuleBlock = $whitespace;


The property description says: "Content injected in and around at-rule blocks." Which suggests it's not necessarily whitespace, and thus the parameter should be differently named.

JakeQZ · 2025-02-06T18:15:37Z

src/OutputFormat.php

+    public function setAfterAtRuleBlock(string $whitespace): self
+    {
+        $this->sAfterAtRuleBlock = $whitespace;


JakeQZ · 2025-02-06T18:39:42Z

src/OutputFormat.php

+    public function setSpaceBeforeListArgumentSeparators(array $separators): self
+    {
+        $this->aSpaceBeforeListArgumentSeparators = $separators;


The separators are the array keys. The values are the space strings. So the parameter name is inaccurate.

In #880 I used $spaceForSeparator as a variable for the array, but am now wondering if one or both nouns should be plural. And I'm also wondering whether setSpaceBeforeListArgumentSeparators() was the best choice of name for the new method, though can't think of anything better.

I've changed it. Please have a look.

JakeQZ · 2025-02-06T18:40:05Z

src/OutputFormat.php

+    public function setSpaceAfterListArgumentSeparators(array $separators): self
+    {
+        $this->aSpaceAfterListArgumentSeparators = $separators;


JakeQZ · 2025-02-06T18:44:08Z

src/OutputFormat.php

+    public function setBeforeDeclarationBlock(string $whitespace): self
+    {
+        $this->sBeforeDeclarationBlock = $whitespace;


The DocBlock for the property says: "Content injected in and around declaration blocks." Which implies it's not necessarily whitespace. (Also, it's not a setSpace... method.)

JakeQZ · 2025-02-06T18:44:18Z

src/OutputFormat.php

+    public function setAfterDeclarationBlockSelectors(string $whitespace): self
+    {
+        $this->sAfterDeclarationBlockSelectors = $whitespace;


JakeQZ · 2025-02-06T18:44:29Z

src/OutputFormat.php

+    public function setAfterDeclarationBlock(string $whitespace): self
+    {
+        $this->sAfterDeclarationBlock = $whitespace;


JakeQZ · 2025-02-06T18:45:51Z

src/OutputFormat.php

+    public function setIndentation(string $sIndentation): self
+    {
+        $this->sIndentation = $sIndentation;


Perhaps avoid the Hungarian notation in the parameter name.

JakeQZ · 2025-02-06T19:08:44Z

src/OutputFormat.php

+    /**
+     * @return $this fluent interface
+     */
+    public function setIndentationLevel(int $indentationLevel): self
+    {
+        $this->iIndentationLevel = $indentationLevel;
+
+        return $this;
+    }


I just noticed that the $oFormatter, $oNextLevelFormat and $iIndentationLevel properties are private (whereas the others are all public), so I'm wondering whether they are intended to be accessed externally. Though currently they all can be via the generic get and set methods.

The OutputFormatter class uses getIndentationLevel(), so that at least is required (but maybe the corresponding setter isn't).

I'd like to make all properties private anyway.

I'd like to make all properties private anyway.

Agree. We can mark them @deprecated to be made private in 9.0, with a message to use the relevant accessor methods. (And when they're made private, the Hungarian notation can be dropped.)

JakeQZ · 2025-02-06T19:14:58Z

tests/OutputFormatTest.php

            '.main, .test {font: italic normal bold 16px/1.2 "Helvetica",	Verdana,	sans-serif;background: white;}'
            . "\n@media screen {.main {background-size: 100% 100%;font-size: 1.3em;background-color: #fff;}}",
-            $this->document->render(OutputFormat::create()->setSpaceAfterListArgumentSeparator([
+            $this->document->render(OutputFormat::create()->setSpaceAfterListArgumentSeparators([


This is intended to test the deprecated functionality, to confirm it still works, and should not be changed. When the deprecated functionality is removed, this test method can also be removed. (Incidentally, with the new functionality, the plural method does not support the first array item being the default - the default is instead set by the existing method with a string parameter. This doesn't make a difference here since the spacing for all separators used in the test has been specified explicitly.)

oliverklee · 2025-02-06T21:12:47Z

I'm wondering if any or all of the private properties should or shouldn't have accessors (but am unsure).

The goal of this PR is to replace the magic getters/setters, which means keeping the same level of access to (or encapsulation of) the OutputFormat properties. I'd like to keep the properties as encapsulated as possible. So, IMHO, they shouldn't have accessors.

As far as our test coverage goes, we currently don't need any additional accessors to the OutputFormat properties.

JakeQZ · 2025-02-06T22:28:07Z

I'm wondering if any or all of the private properties should or shouldn't have accessors (but am unsure).

The goal of this PR is to replace the magic getters/setters, which means keeping the same level of access to (or encapsulation of) the OutputFormat properties. I'd like to keep the properties as encapsulated as possible. So, IMHO, they shouldn't have accessors.

That was my thinking. Though the private $iIndentationLevel needed a 'getter' for the OutputFormatter class. Beyond the scope of this PR, but I wondered on the preceding basis if it really needed a 'setter' as well.

oliverklee · 2025-02-06T22:34:36Z

Though the private $iIndentationLevel needed a 'getter' for the OutputFormatter class. Beyond the scope of this PR, but I wondered on the preceding basis if it really needed a 'setter' as well.

So far, the setter only used in tests. So I think we could indeed drop it again. What do you think?

oliverklee · 2025-02-06T22:36:46Z

Removed setIndentationLevel and repushed.

JakeQZ · 2025-02-06T22:45:57Z

Though the private $iIndentationLevel needed a 'getter' for the OutputFormatter class. Beyond the scope of this PR, but I wondered on the preceding basis if it really needed a 'setter' as well.

So far, the setter only used in tests. So I think we could indeed drop it again. What do you think?

I could be wrong, but I don't think it's needed in the public API - it's for internal use rendering nested items.

We'll need to adjust (or remove) the test which uses it though.

oliverklee added the enhancement label Feb 2, 2025

oliverklee added this to the 9.0.0: Drop support for PHP < 7.2 and clean up things milestone Feb 2, 2025

oliverklee self-assigned this Feb 2, 2025

oliverklee marked this pull request as draft February 2, 2025 17:48

oliverklee force-pushed the feature/property-accessors branch 4 times, most recently from 80236b4 to 65afd70 Compare February 3, 2025 10:42

oliverklee mentioned this pull request Feb 5, 2025

[TASK] Deprecate sSpaceAfterListArgumentSeparator being an array #876

Closed

oliverklee changed the title ~~[FEATURE] Add dedicated accessors for OutputFormat~~ [FEATURE] Add dedicated property accessors for OutputFormat Feb 6, 2025

oliverklee force-pushed the feature/property-accessors branch from 65afd70 to d7d4a44 Compare February 6, 2025 16:29

oliverklee marked this pull request as ready for review February 6, 2025 16:29

[FEATURE] Add dedicated property accessors for OutputFormat

456e4d5

oliverklee force-pushed the feature/property-accessors branch from d7d4a44 to 456e4d5 Compare February 6, 2025 16:31

oliverklee requested review from JakeQZ and sabberworm February 6, 2025 16:33

JakeQZ requested changes Feb 6, 2025

View reviewed changes

oliverklee force-pushed the feature/property-accessors branch from 456e4d5 to c566b31 Compare February 6, 2025 21:16

Changes suggested in code review

5d53656

oliverklee force-pushed the feature/property-accessors branch from c566b31 to 5d53656 Compare February 6, 2025 22:32

oliverklee requested a review from JakeQZ February 6, 2025 22:32

Drop setIndentationLevel() again

ec1aa5a

JakeQZ approved these changes Feb 6, 2025

View reviewed changes

JakeQZ merged commit 88b8729 into main Feb 6, 2025
21 checks passed

JakeQZ deleted the feature/property-accessors branch February 6, 2025 22:41

Uh oh!

[FEATURE] Add dedicated property accessors for OutputFormat #871

[FEATURE] Add dedicated property accessors for OutputFormat #871

Uh oh!

Conversation

oliverklee commented Feb 2, 2025

Uh oh!

coveralls commented Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliverklee commented Feb 6, 2025

Uh oh!

JakeQZ commented Feb 6, 2025

Uh oh!

oliverklee commented Feb 6, 2025

Uh oh!

oliverklee commented Feb 6, 2025

Uh oh!

Uh oh!

JakeQZ commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[FEATURE] Add dedicated property accessors for `OutputFormat` #871

[FEATURE] Add dedicated property accessors for `OutputFormat` #871

coveralls commented Feb 2, 2025 •

edited

Loading