Skip to content
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

NotOperatorWithSpaceFixer - NotOperatorWithSuccessorSpaceFixer - depr… #4735

Closed
wants to merge 1 commit into from
Closed

NotOperatorWithSpaceFixer - NotOperatorWithSuccessorSpaceFixer - depr… #4735

wants to merge 1 commit into from

Conversation

SpacePossum
Copy link
Contributor

closes

#4674

@@ -1851,6 +1852,12 @@ Choose from the list of available rules:

Unary operators should be placed adjacent to their operands.

Configuration options:

- ``not_operator_space`` (``'leading_and_trailing'``, ``'no_trailing'``,
Copy link
Contributor

Choose a reason for hiding this comment

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

This got me confused as the new user of PHP CS Fixer - why only those 3 options? Where are no_leading_and_trailing, no_leading and one_leading options? Are they "worse" then added 3?

Wouldn't be more clear to have 2 boolean options leading and trailing with value true|false|null to cover all combination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the current logic of the fixers into one and made the difference in behavior available through these configuration options. I didn't spend time on investigating new or better options, so thanks for the suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, and no_trailing is the default for new fixer, I missed that, now I get it 👍

@@ -48,7 +50,15 @@ public function getDefinition()
*/
public function getPriority()
Copy link
Member

Choose a reason for hiding this comment

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

This method can be dropped now.

@@ -48,7 +50,15 @@ public function getDefinition()
*/
public function getPriority()
Copy link
Member

Choose a reason for hiding this comment

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

This method can be dropped now.

{
return new FixerConfigurationResolver(
[
(new FixerOptionBuilder('not_operator_space', 'Space around logical `!` (`not`) operators.'))
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to have an operators option supporting all possible operators like binary_operator_spaces has?

@julienfalque julienfalque modified the milestones: 2.15.6, 2.17.0 Feb 2, 2020
@julienfalque
Copy link
Member

@SpacePossum deprecations cannot be added to a patch version, this must be merged into master.

@SpacePossum
Copy link
Contributor Author

Im pushing this back to the discussion about the request so we can agree on what to do there, thanks all for the reviewing!

@SpacePossum SpacePossum closed this Feb 2, 2020
@SpacePossum SpacePossum deleted the 2_15_4674_merge_not_op branch February 2, 2020 14:03
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