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

LineEndingFixer - handle "\r\r\n" #4699

Merged
merged 1 commit into from
Jan 7, 2020
Merged

LineEndingFixer - handle "\r\r\n" #4699

merged 1 commit into from
Jan 7, 2020

Conversation

kubawerlos
Copy link
Contributor

The added test is converting the input to expected, but it fails because:

Code build on input code must match expected code.

We have two ways to fix it:

  1. Update the RegEx #\r\n|\n# to #\r+\n|\n#
  2. Update the RegEx #\r\n|\n# to #\r\n|\r|\n# and test case to:
        $cases[] = [
            "<?php echo 'foo',\n\n'bar';",
            "<?php echo 'foo',\r\r\n'bar';",
        ];

Option 2 seems more right as the original \r\r\n is treated (at least in PHPStorm) as 2 line endings.

Ping @fabpot, @SpacePossum and @keradus as authors for opinion.

@julienfalque
Copy link
Member

What about #\R#?

@kubawerlos
Copy link
Contributor Author

Even better :)

@julienfalque
Copy link
Member

I'm not against changes proposed here but, if I remember correcly, the current behavior (not supporting \r) is on purpose because the syntax is only used on old systems.

@kubawerlos
Copy link
Contributor Author

The problem is that the current behavior is wrong - fixer cannot leave the code that requires fixing by itself.

@SpacePossum
Copy link
Contributor

Not sure if we left \r support in/out by design, or didn't bother trying to fix it because it is only used on older setups as we are talking about before macOS X which was released in 2002.

With the proposed changes in this PR, which users might be effected in a way that we might not want?

@kubawerlos
Copy link
Contributor Author

@SpacePossum not sure, some that have really old piece of code I guess, I doubt anyone is using \r in new code.

@SpacePossum
Copy link
Contributor

Can you propose the fix in the PR in a new commit? I think it will be fine TBH

@julienfalque julienfalque added this to the 2.15.6 milestone Jan 6, 2020
@SpacePossum SpacePossum added the RTM Ready To Merge label Jan 7, 2020
@SpacePossum SpacePossum changed the title [RFC] LineEndingFixer - handle "\r\r\n" LineEndingFixer - handle "\r\r\n" Jan 7, 2020
@SpacePossum SpacePossum removed the RTM Ready To Merge label Jan 7, 2020
@SpacePossum
Copy link
Contributor

Thank you @kubawerlos.

SpacePossum added a commit that referenced this pull request Jan 7, 2020
This PR was squashed before being merged into the 2.15 branch (closes #4699).

Discussion
----------

LineEndingFixer - handle "\r\r\n"

The added test is converting the input to expected, but it fails because:

> Code build on input code must match expected code.

We have two ways to fix it:
1. Update the RegEx `#\r\n|\n#` to `#\r+\n|\n#`
2. Update the RegEx `#\r\n|\n#` to `#\r\n|\r|\n#` and test case to:
```php
        $cases[] = [
            "<?php echo 'foo',\n\n'bar';",
            "<?php echo 'foo',\r\r\n'bar';",
        ];
```

Option 2 seems more right as the original `\r\r\n` is treated (at least in PHPStorm) as 2 line endings.

Ping @fabpot, @SpacePossum and @keradus as authors for opinion.

Commits
-------

b465a72 LineEndingFixer - handle \"\r\r\n\"
@SpacePossum SpacePossum merged commit b465a72 into PHP-CS-Fixer:2.15 Jan 7, 2020
@kubawerlos kubawerlos deleted the LineEndingFixer_double_backshash_r branch January 7, 2020 15:46
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.

None yet

3 participants