-
Notifications
You must be signed in to change notification settings - Fork 150
[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
Conversation
JakeQZ
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OutputFormat::spaceAfterListArgumentSeparatoris 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.
6e88a94 to
4a959bc
Compare
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:
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`.
4a959bc to
7b11d2a
Compare
|
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 Then it would be possibke to deprecate the array parameter option to have different spacing depending on list separator, with e.g. 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? |
`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.
OutputFormat::spaceAfterListArgumentSeparatoris expected to be a string, not an array.This was discovered by adding dedicated, strictly typed property accessors for
OutputFormat.