-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Don't use POSIX-specific shell constructs #436
Conversation
This is part of work to bring 'shards' to Windows.
@@ -113,7 +113,7 @@ module Shards | |||
|
|||
protected def self.has_git_command? | |||
if @@has_git_command.nil? | |||
@@has_git_command = Process.run("command -v git", shell: true).success? | |||
@@has_git_command = (Process.run("git", ["--version"]).success? rescue false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@has_git_command = (Process.run("git", ["--version"]).success? rescue false) | |
@@has_git_command = Process.run("git", ["--version"]).success? rescue false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can work both ways but just consider what advantages we're weighing here (picking one or the other):
- Saving two characters.
- Not having to think what the rescue really applies to every time when reading the code. I literally didn't know what the priority would be and I wanted to save everyone the cognitive load and the possible bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I thought it's pretty clear what's going on here (there's nothing else that 'rescue' could apply to), but it's your call.
PS. for me the cognitive load is asking myself why did u do that in the first place ;)
@bcardiff have you had a chance to look at my pull requests to Shards? |
This is part of work to bring 'shards' to Windows.
This is part of work to bring 'shards' to Windows.