Skip to content

[BUGFIX] Drop test that passes in the wrong type #866

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

Closed
wants to merge 1 commit into from

Conversation

oliverklee
Copy link
Collaborator

OutputFormat::spaceAfterListArgumentSeparator is expected to be a string, not an array.

This was discovered by adding dedicated, strictly typed property accessors for OutputFormat.

@coveralls
Copy link

coveralls commented Feb 2, 2025

Coverage Status

coverage: 45.742%. remained the same
when pulling 7b11d2a on bugfix/space-type
into cde73cd 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.

OutputFormat::spaceAfterListArgumentSeparator is expected to be a string, not an array.

I think it is expected to be a string or an array. If an array, the keys are list separators and the values are the space string to insert after them, the the first item in the array (also) serving as the default space separator. See OutputFormat::createPretty() for examples and OutputFormmatter::space() for implementation.

@oliverklee
Copy link
Collaborator Author

I think it is expected to be a string or an array.

Yes, that's how it currently works (even if it violates the type annotation). This behavior is not documented, and I'd like to get rid of it.

@oliverklee oliverklee requested a review from JakeQZ February 2, 2025 17:52
@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 2, 2025

I think it is expected to be a string or an array.

Yes, that's how it currently works (even if it violates the type annotation). This behavior is not documented, and I'd like to get rid of it.

It's documented by this:

To see what you can do with output formatting, look at the tests in tests/OutputFormatTest.php.

It's also part of the public API. So if we remove it we'll first need to mark it as deprecated.

`OutputFormat::spaceAfterListArgumentSeparator` is expected to
be a string, not an array.

This was discovered by adding dedicated, strictly typed
property accessors for `OutputFormat`.
@oliverklee oliverklee marked this pull request as draft February 2, 2025 22:02
@JakeQZ
Copy link
Collaborator

JakeQZ commented Feb 3, 2025

If we do want to remove the functionality, we'd first need to create the dedicated (non-majic) methods (which @oliverklee I see you're on a path towards, e.g. with #867), whilst retaining current functionality, and correcting the type definitions to include array as a possibility.

Then it would be possibke to deprecate the array parameter option to have different spacing depending on list separator, with e.g. @depracated From 10.0 the $space parameter can no longer be an array.

Having done the first part, it would be extra effort to remove the feature. I think we may as well retain it. @oliverklee, @sabberworm, WDYT?

JakeQZ added a commit that referenced this pull request Feb 6, 2025
`SpaceBeforeListArgumentSeparators` and `SpaceAfterListArgumentSeparators`
are added to allow for different spacing for different separators.

`SpaceBeforeListArgumentSeparator` and `SpaceAfterListArgumentSeparator`
will specify the default spacing.  Setting these as an array is deprecated.

Resolves #866, #876, #878.
@oliverklee oliverklee closed this in 7a12457 Feb 6, 2025
@oliverklee oliverklee deleted the bugfix/space-type branch February 6, 2025 16:18
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.

3 participants