-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
fixes #70 |
(candidates find (_.name == name) orElse candidates.headOption) map (_.path) | ||
val plain = candidates find (_.name == name) | ||
val exe = candidates find (_.name == s"${name}.exe") | ||
(plain orElse exe orElse candidates.headOption).map(_.path) |
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.
Is it plainer to do val realname = if (util.Properties.isWin && !name.endsWith(".exe")) s"$name.exe" else name
and find that? Add case-insensitive test, of course. I was a little clever with the dot test to avoid these platform dances.
Actually the filter should read e.name == name || isWin && e.name.looksLike(name.exe)
. Then assert there's at most one candidate, with a nice message, to avoid future confusion.
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.
In practice, if there is neither a name
or name.exe
, the guess is most likely just wrong. The theoretical platform/jdk/jre that uses the same name but a different extension probably doesn't exist, and abstracting over that hypothetical probably isn't worth the brain cycles.
replacing the whole thing with something like
(p walkFilter { e => (e.name == name || e.name == e.name.looksLike(s"${name}.exe") && e.jfile.canExecute }).map(_.path).headOption
or even just
((Path(jdkHome) / "bin") .walkFilter { e => (e.name == name || e.name == s"${name}.exe") && e.jfile.canExecute }).map(_.path).headOption
should work fine enough. It's not going to find java.woozle
if that is the correct file (which it isn't). If it's looking for a file named bin
(which it isn't) and jdkhome/bin
is a file rather than a directory (which it also isn't) it's going to return the path of jdkhome/bin
rather than None
mistakenly. I'm not sure that would be a great loss.
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.
Yes, even simpler.
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.
I'll update the PR to whichever is your preference when I'll get home tonight
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.
Your second expr gets it done. With paranoid case insensitive.
(Path(jdkHome) / "bin").walk.find(e => e.name == name || isWin && e.name.equalsIgnoreCase(s"$name.exe")).map(_.path)
When I looked yesterday, the doc on walkFilter (there's doc) inverts the sense of filter, so I wouldn't assume anything about experimental i/o library. But walk on a regular file seems OK.
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.
I wouldn't assume anything about the doc either:
/** Equivalent to walkFilter(_ => false).
*/
def walk: Iterator[Path] = walkFilter(_ => true)
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 missed that one. That's so funny.
LGTM I left a comment but it's 6/13 of a baker's dozen. |
FTR, |
If only the Scala team had a Windows expert on staff. |
fe3b3fb
to
aaadfd7
Compare
canExecute does not guarantee an executable file, and will find java.dll before java.exe on Windows
aaadfd7
to
70f82d8
Compare
thanks Martijn! |
manually check for a .exe
manually check for a .exe
canExecute does not guarantee an executable file, and will find java.dll before java.exe on Windows