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

NoBlankLines*: fix removing lines consisting only of spaces #4109

Merged
merged 1 commit into from Dec 28, 2018
Merged

NoBlankLines*: fix removing lines consisting only of spaces #4109

merged 1 commit into from Dec 28, 2018

Conversation

kubawerlos
Copy link
Contributor

We have 3 NoBlankLines* fixers and they behave differently for lines consisting only of spaces:

  • NoBlankLinesAfterClassOpeningFixer - removes line of spaces only when next line is indented
  • NoBlankLinesAfterPhpdocFixer - same as above
  • NoBlankLinesBeforeNamespaceFixer - removes indentation of the next line

What would be the proper behaviour (apart of removing indentation of next line what is obviously a bug) - to keep lines consisting only of spaces or to remove them?

Tests are to demonstrate the problem, obviously have to be fixed as well.

@SpacePossum
Copy link
Contributor

how about merging those into no_extra_blank_lines ?

@dmvdbrugge
Copy link
Contributor

dmvdbrugge commented Nov 15, 2018

how about merging those into no_extra_blank_lines ?

There's a difference between "no extra" (meaning at most 1) or "no" (meaning 0).

I'd be for combining the ones pointed out into a single fixer (just like the blank_line_before_statement), however think that it should be a new one. (With the deprecations ofc.)

@SpacePossum
Copy link
Contributor

if I expect 0 than 1 is extra to me

@dmvdbrugge
Copy link
Contributor

But how are you going to configure where you expect zero and where one? Sounds like configuration hell. I for one, wouldn't want NoBlankLinesBeforeNamespaceFixer while I do want the others.

@kubawerlos
Copy link
Contributor Author

Okay, as there obviously going to be discussion about new fixer to remove blank lines let's focus here about fixing the bugs only.

@kubawerlos kubawerlos changed the title [RFC] NoBlankLines* inconsistency NoBlankLines*: fix removing lines consisting only of spaces Nov 16, 2018
@julienfalque julienfalque added this to the 2.12.5 milestone Nov 30, 2018
@SpacePossum SpacePossum added the RTM Ready To Merge label Dec 27, 2018
@@ -1,22 +0,0 @@
--TEST--
Copy link
Member

@keradus keradus Dec 28, 2018

Choose a reason for hiding this comment

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

please do not remove tests.
ever

you found out nice optimization? cool, apply it and leave the test present, so during refactoring year later, it will still work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, I haven't thought about moving it from priority to misc. Next time I will remember.

@keradus
Copy link
Member

keradus commented Dec 28, 2018

Thank you @kubawerlos.

@keradus keradus merged commit 7df251c into PHP-CS-Fixer:2.12 Dec 28, 2018
keradus added a commit that referenced this pull request Dec 28, 2018
… (kubawerlos, keradus)

This PR was squashed before being merged into the 2.12 branch (closes #4109).

Discussion
----------

NoBlankLines*: fix removing lines consisting only of spaces

We have 3 `NoBlankLines*` fixers and they behave differently for lines consisting only of spaces:
- `NoBlankLinesAfterClassOpeningFixer` - removes line of spaces only when next line is indented
- `NoBlankLinesAfterPhpdocFixer` - same as above
- `NoBlankLinesBeforeNamespaceFixer` - removes indentation of the next line

What would be the proper behaviour (apart of removing indentation of next line what is obviously a bug) - to keep lines consisting only of spaces or to remove them?

_Tests are to demonstrate the problem, obviously have to be fixed as well._

Commits
-------

7df251c NoBlankLines*: fix removing lines consisting only of spaces
@keradus keradus removed the RTM Ready To Merge label Dec 28, 2018
@kubawerlos kubawerlos deleted the bugfix/no-blank-lines-inconsistency branch December 29, 2018 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants