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

Fix broken indention on Windows systems because of line endings #4221

Merged
merged 1 commit into from Jul 13, 2017

Conversation

5 participants
@electrokit

electrokit commented Jul 12, 2017

For issue #4220
This isn't tested, except in my home environment. The super-critical parts are
php/WP_CLI/Dispatcher/CommandFactory.php
php/WP_CLI/Dispatcher/Subcommand.php:30 preg_match_all( '/(.+?)\R+:/', $longdesc, $matches );
php/WP_CLI/DocParser.php:73

This also fixes the wp help command, which screwed up indentation on win systems, regardless of line endings in file.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 12, 2017

Member

Thanks for the pull request, @electrokit. I appreciate your effort on it.

At this point, I'm 👎 on merging this for a couple of reasons:

  1. WP-CLI doesn't formally support Windows because we have no way of running automated tests in a Windows environment. Even if we were able to attain compatibility now, we'd likely regress in the future.
  2. With such broad sweeping changes, there's the likelihood of things breaking (as you've already discovered). Even if you get the tests passing, not everything has test coverage, so it's possible something else has broken silently.
Member

danielbachhuber commented Jul 12, 2017

Thanks for the pull request, @electrokit. I appreciate your effort on it.

At this point, I'm 👎 on merging this for a couple of reasons:

  1. WP-CLI doesn't formally support Windows because we have no way of running automated tests in a Windows environment. Even if we were able to attain compatibility now, we'd likely regress in the future.
  2. With such broad sweeping changes, there's the likelihood of things breaking (as you've already discovered). Even if you get the tests passing, not everything has test coverage, so it's possible something else has broken silently.
@szepeviktor

This comment has been minimized.

Show comment
Hide comment
@szepeviktor

szepeviktor Jul 12, 2017

Contributor

Back when there were typewriters CR was moving the cartridge, LF was moving the paper.
Then in computers it was: LF for UNIX, CR for Apple and CRLF for Microsoft.
Then Apple realized LF makes sense.

Nowaday there is "Ubuntu for Windows" https://msdn.microsoft.com/en-us/commandline/wsl/about
In couple of years Windows will support LF - I think...

Contributor

szepeviktor commented Jul 12, 2017

Back when there were typewriters CR was moving the cartridge, LF was moving the paper.
Then in computers it was: LF for UNIX, CR for Apple and CRLF for Microsoft.
Then Apple realized LF makes sense.

Nowaday there is "Ubuntu for Windows" https://msdn.microsoft.com/en-us/commandline/wsl/about
In couple of years Windows will support LF - I think...

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Jul 13, 2017

Contributor

That last commit is the way to go, following the 2 golden rules in these situations:

  1. Normalize your input as early as possible.
  2. Denormalize your output as late as possible.

In between you just deal with the normalized input, ie \n in this case. So PHP_EOL and \r shouldn't be used, except at the very beginning/end.

I couldn't resist doing this, see gitlost#14, although it will probably go nowhere. It'd be interesting anyway if you @electrokit could give it a bash (do you have the behat tests running on Windows?).

Contributor

gitlost commented Jul 13, 2017

That last commit is the way to go, following the 2 golden rules in these situations:

  1. Normalize your input as early as possible.
  2. Denormalize your output as late as possible.

In between you just deal with the normalized input, ie \n in this case. So PHP_EOL and \r shouldn't be used, except at the very beginning/end.

I couldn't resist doing this, see gitlost#14, although it will probably go nowhere. It'd be interesting anyway if you @electrokit could give it a bash (do you have the behat tests running on Windows?).

@electrokit

This comment has been minimized.

Show comment
Hide comment
@electrokit

electrokit Jul 13, 2017

@danielbachhuber, is the update acceptable? The two changes should not do anything different in *nix environments with files with LF line endings.

electrokit commented Jul 13, 2017

@danielbachhuber, is the update acceptable? The two changes should not do anything different in *nix environments with files with LF line endings.

@danielbachhuber danielbachhuber added this to the 1.3.0 milestone Jul 13, 2017

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 13, 2017

Member

@electrokit Yes, I'm fine with this pull request as it stands now.

Member

danielbachhuber commented Jul 13, 2017

@electrokit Yes, I'm fine with this pull request as it stands now.

@danielbachhuber danielbachhuber merged commit 2b9efb1 into wp-cli:master Jul 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber changed the title from Fixed line endings to Fix broken indention on Windows systems because of line endings Jul 13, 2017

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Jul 13, 2017

Contributor
$docComment = preg_replace( '/\R/', "\n", $docComment );

should just be

$docComment = str_replace( "\r\n", "\n", $docComment );
Contributor

gitlost commented Jul 13, 2017

$docComment = preg_replace( '/\R/', "\n", $docComment );

should just be

$docComment = str_replace( "\r\n", "\n", $docComment );
@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 13, 2017

Member

@gitlost Can you submit in a new PR?

Member

danielbachhuber commented Jul 13, 2017

@gitlost Can you submit in a new PR?

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Jul 13, 2017

Contributor

Will do...

Contributor

gitlost commented Jul 13, 2017

Will do...

@electrokit

This comment has been minimized.

Show comment
Hide comment
@electrokit

electrokit Jul 13, 2017

$docComment = str_replace( "\r\n", "\n", $docComment );
will break on e.g. Mac OS 9. Maybe not a problem because of the performance increase?

electrokit commented Jul 13, 2017

$docComment = str_replace( "\r\n", "\n", $docComment );
will break on e.g. Mac OS 9. Maybe not a problem because of the performance increase?

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Jul 13, 2017

Contributor

Ha! Also on ZX Spectrums... curious, do you have the behat tests running on Windows?

Contributor

gitlost commented Jul 13, 2017

Ha! Also on ZX Spectrums... curious, do you have the behat tests running on Windows?

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 14, 2017

Member

@electrokit I don't quite understand the Mac OS 9 comment. Was that meant to be a joke or does it reflect a serious consideration?

Member

danielbachhuber commented Jul 14, 2017

@electrokit I don't quite understand the Mac OS 9 comment. Was that meant to be a joke or does it reflect a serious consideration?

@gfolin

This comment has been minimized.

Show comment
Hide comment
@gfolin

gfolin Jul 14, 2017

Contributor

Mostly a joke, just that I think that \R results in nicer code (removes the definition of newlines away from the code). But for performance reasons alone (around a factor 2), I think the code that @gitlost wrote is better.

Contributor

gfolin commented Jul 14, 2017

Mostly a joke, just that I think that \R results in nicer code (removes the definition of newlines away from the code). But for performance reasons alone (around a factor 2), I think the code that @gitlost wrote is better.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jul 14, 2017

Member

👍 Thanks @gfolin

Member

danielbachhuber commented Jul 14, 2017

👍 Thanks @gfolin

@gfolin

This comment has been minimized.

Show comment
Hide comment
@gfolin

gfolin Jul 15, 2017

Contributor

Just so everyone knows, I'm @electrokit as well. Work account.

Contributor

gfolin commented Jul 15, 2017

Just so everyone knows, I'm @electrokit as well. Work account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment