8000 Replace hardcode linefeed by TylerLeonhardt · Pull Request #11746 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@TylerLeonhardt
Copy link
Member
@TylerLeonhardt TylerLeonhardt commented Jan 31, 2020

PR Summary

I've moved it over to using Environment.NewLine. I added a test to test this specific thing... but maybe in the future we can put that test in a "crossplat" test file that checks all of our ToString's to make sure they honor the newline for the platform.

PR Context

There was a hardcoded line feed in the ToString method of ParseError. this caused a CI test failure in the Jupyter kernel because on Windows, there would be all \r\n except for this single instance of \n.

Tooling could be impacted, but from my look around the tooling code, I don't think it would be.

This is a breaking change, albeit small... and worth it IMO.

PR Checklist

@TylerLeonhardt
Copy link
Member Author

@PoshChan please retry macos

@PoshChan
Copy link
Collaborator

@TylerLeonhardt, successfully started retry of PowerShell-CI-macOS

Copy link
Member
@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

@rjmholt Any possibility this simple change could cause a regression?

@daxian-dbw daxian-dbw requested a review from rjmholt January 31, 2020 19:18
At line:1 char:5
+ $x =
+ ~
You must provide a value expression following the '=' operator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Localized message is not good in test. We could remove it and check that right newline is after the extend.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworked the test.

@rjmholt
Copy link
Collaborator
rjmholt commented Jan 31, 2020

Any possibility this simple change could cause a regression?

I don't know of anything that depends on the ToString() behaviour of this class. Certainly nothing should.

@daxian-dbw daxian-dbw merged commit ac1b83f into PowerShell:master Feb 3, 2020
@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Feb 4, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Feb 4, 2020
@daxian-dbw
Copy link
Member

Any possibility this simple change could cause a regression?

I don't know of anything that depends on the ToString() behaviour of this class. Certainly nothing should.

Guess what, our error view depends on this ToString() behavior 😄 This PR accidentally fixed a regression in the concise view for parsing errors.

@rjmholt
Copy link
Collaborator
rjmholt commented Mar 4, 2020

our error view depends on this ToString() behavior

🤦‍♂

@TylerLeonhardt TylerLeonhardt deleted the replace-hardcode-linefeed branch March 4, 2020 21:07
@ghost
Copy link
ghost commented Mar 26, 2020

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@TravisEz13 TravisEz13 modified the milestones: 7.0.1-approved, 7.0.1 Apr 20, 2020
silijon pushed a commit to SkyKick/PowerShell that referenced this pull request Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

0