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

Detect windows using os.name rather than line.separator. #639

Merged
merged 7 commits into from
Jul 4, 2020

Conversation

nedtwigg
Copy link
Member

@nedtwigg nedtwigg commented Jul 3, 2020

Added FileSignature.machineIsWin, which detects the OS using os.name, and replaces the now-deprecated LineEnding.nativeIsWin, which detected the OS using line.separator.

…orm is unreliable. Deprecate `LineEnding.nativeIsWin()`, and add `FileSignature.machineIsWin()`.
…machineIsWin()`. Notably, none of the usages actually cared about line endings, they only cared about paths and default system shells, which indicates that bundling this with `LineEnding` was probably always a bad idea.
Copy link
Contributor

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

This should work in any case and the performance penalty on *nix machines is probably neglegible. Even if you keep the check (probably a !machineIsUnix() would be more clear, since that is essentially what is interesting here) you could keep the File.separatorChar.

nedtwigg and others added 2 commits July 4, 2020 09:31
Co-authored-by: J-N-K <J-N-K@users.noreply.github.com>
Co-authored-by: J-N-K <J-N-K@users.noreply.github.com>
@nedtwigg
Copy link
Member Author

nedtwigg commented Jul 4, 2020

Ooh, I like the File.separatorChar, thanks. We'll have a bugfix release out by Monday, there's one other bug I need to look into.

@nedtwigg
Copy link
Member Author

nedtwigg commented Jul 4, 2020

There was a related bug, which was completely clobbering the git-native line endings anyway. Just fixed in previous commit. With these fixes combined, I think we have fixed the problem completely.

@nedtwigg nedtwigg merged commit b77fef2 into main Jul 4, 2020
@nedtwigg nedtwigg deleted the feat/detect-windows branch July 4, 2020 21:24
@nedtwigg
Copy link
Member Author

nedtwigg commented Jul 5, 2020

Released in plugin-maven 2.0.1 and plugin-gradle 4.5.1

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.

2 participants