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

Return error if color property or value is invalid with Set-PSReadLineOption -Colors #1124

Merged
merged 3 commits into from Nov 8, 2019

Conversation

SteveL-MSFT
Copy link
Member

Add descriptive error messages if the color property or value is invalid. Had to rerun resgen regenerating the resources cs file.

Fix #1118

Copy link
Member

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

PSReadLineResources.Designer.cs seems to have changed every line - probably a CRLF => LF change?

@SteveL-MSFT
Copy link
Member Author

@lzybkr ran resgen.exe on Windows. VSCode probably changed it. I can change it back to CRLF.

@SteveL-MSFT
Copy link
Member Author

Actually, resgen creates a file with CRLF. Resaved as LF only. VSCode cleaned up trailing whitespace.

@SteveL-MSFT
Copy link
Member Author

@lzybkr can you re-review? Thanks.

lzybkr
lzybkr previously approved these changes Oct 28, 2019
Copy link
Member

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

Approved, assuming resgen removed the trailing whitespace.
If the trailing whitespace was removed by an editor, then it's probably better to keep it or somebody else will just end up adding it back in another PR.

@lzybkr lzybkr dismissed their stale review October 28, 2019 20:31

Whitespace removed by editor

lzybkr
lzybkr previously requested changes Oct 28, 2019
Copy link
Member

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

Since resgen is apparently generating trailing whitespace, we should remove it as future changes will likely add it back.

@SteveL-MSFT
Copy link
Member Author

@lzybkr it seems that resgen has some hardcoded whitespace and also generates CRLF by default. git saves files as LF. In the future, we should probably have resgen as part of the build script if resources change. I don't see a need value in putting in the "raw" resgen'd file unless we also want the CRLF line endings.

@lzybkr
Copy link
Member

lzybkr commented Oct 29, 2019

Maybe it's best to run resgen as part of the build and remove the generated file. It should be much simpler now than when this project was started.

@SteveL-MSFT
Copy link
Member Author

@lzybkr if we depend on resgen.exe, then builds only work on Windows again

@lzybkr
Copy link
Member

lzybkr commented Oct 29, 2019

I guess I'm not following what you're suggesting then. But I think my point still stands - we don't want unnecessary changes to this file as it is generated - those changes are too easily undone and make reviews harder than necessary.

git will do what you tell it to w.r.t. eol, so we could add a .gitattributes file with a line like:

PSReadLine/PSReadLineResources.Designer.cs eol=crlf

And I think that wouldn't change what's in the index (which is lf for this file, but crlf and even mixed for other files in this repo).

@SteveL-MSFT
Copy link
Member Author

@lzybkr the file in git is currently LF which is why I had to resave it from CRLF to just LF so that the entire file didn't show up as modified. However, resgen (being a Windows tool) emits CRLF by default. So at some point in the past that file was already resaved as just LF. My suggestion is to defer fixing the resgen issue to later if we have to make more changes to the resx. Perhaps publish the resgen we have in PS repo and use that.

@daxian-dbw
Copy link
Member

@SteveL-MSFT after reading the conversations here, it looks to me what @lzybkr asked was to keep the trailing spaces in the generated file.
If you don't mind, I can re-generate the strongly-typed resource class file, change CRLF to LF and keep the trailing spaces.

@daxian-dbw daxian-dbw dismissed lzybkr’s stale review November 8, 2019 00:58

Trailing spaces are retained now.

@daxian-dbw daxian-dbw merged commit 09300c0 into PowerShell:master Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad error message
3 participants