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

Added Properties.isLinux to compliment the isWin and isMac methods #4934

Merged
merged 1 commit into from Mar 1, 2016
Merged

Added Properties.isLinux to compliment the isWin and isMac methods #4934

merged 1 commit into from Mar 1, 2016

Conversation

Shadow53
Copy link
Contributor

Noticed that there was a missing isLinux (or isNix?) method in scala.util.Properties to go with Mac and Linux. The added one checks for os.name startsWith "Linux", which was the case on my Arch Linux system. I ran ant opt-test (after opt-build) but it failed with an OutOfMemoryError, which didn't seem like it could even be related to this, so I assumed that it was a problem from elsewhere.

This is my first-ever pull request, so please go easy :D

@scala-jenkins scala-jenkins added this to the 2.12.0-M4 milestone Jan 31, 2016
@som-snytt
Copy link
Contributor

s/compliment/complement

@He-Pin
Copy link
Contributor

He-Pin commented Feb 1, 2016

how about return a Platform instance?

sealed trait Platform

object Platform{
case object Linux extends Platform
case object Windows extends Platform
...
}

@SethTisue
Copy link
Member

I'm not sure this is a desirable addition. Why would we need to detect Linux specifically, as opposed to Unixy, non-Mac, non-Windows platforms in general? Normally code that uses flags like these is carving the OS world up into three areas: Mac, Windows, Linux/Unix/other.

how about return a Platform instance

if we were doing this over again, or if we planned to enumerate lots of platforms, that might be a better choice, but I don't think it's worth redoing — especially in light of my comments above.

@Shadow53
Copy link
Contributor Author

Shadow53 commented Feb 1, 2016

Honestly I'd be okay with having some sort of isNix, or whatever it would be called to detect Linux/Unix/etc. I don't run any sort of BSD or other Unixy-but-not-Linux system, so I don't know what differs between the two or if it is enough to detect separately. For my own use, however, being able to detect at least a Unixy system is what I was looking for, since I run Arch Linux.

I know that I could write something like

if (!Properties.isWin && !Properties.isMac)

But having an actual isLinux/Nix would be better, IMO.

Perhaps if I made a new pull request for a generalized isNix?

s/compliment/complement

Wow, usually I'm enough of a grammar freak to catch things like that...

@som-snytt
Copy link
Contributor

For namespace, it's also possible to locally

scala> implicit class nix(p: scala.util.Properties.type) {
     | def isNixon = !(p.isWin || p.isMac)
     | }
defined class nix

scala> util.Properties.isNixon
res0: Boolean = true

@SethTisue
Copy link
Member

Perhaps if I made a new pull request for a generalized isNix

Hmm... isGeneric? isOther?

@lrytz
Copy link
Member

lrytz commented Feb 10, 2016

it seems to me isLinux would make more sense than isNix, because it would live at the same level as the existing two, so the three would be mutually exclusive. isNix on the other hand would include isMac (https://en.wikipedia.org/wiki/Unix-like), so i think adding this would be more confusing.

but i'm also in favor to leave things as they are now; when would you use isLinux? most likely when you want !(p.isWin || p.isMac).

@Shadow53
Copy link
Contributor Author

isLinux would definitely make more sense in my book. Having isOther would be akin to !(isWin || isMac || isLinux || ...) for whatever systems would be supported at the time and I think would only be used as a final else when checking for different systems.

but i'm also in favor to leave things as they are now; when would you use isLinux? most likely when you want !(p.isWin || p.isMac).

It's a matter of ease of use and readability, really. I use Linux as my daily driver and am writing code for Linux. As such, I'd like to be able to do a check to make sure my .jar is running on Linux before doing Linux-file-structure-related tasks, in this case editing the hosts file. I know hosts is in the same location on Mac and UNIXy systems, but I plan on adding more that I believe would be different across Linux and BSD systems (I believe FreeBSD has Scala as well). Also, it makes more sense/reads better to use isLinux to check for Linux specifically rather than checking for not Windows or Mac.

@lrytz
Copy link
Member

lrytz commented Feb 15, 2016

OK, so that's what I was wondering: you are actually interested to know if you're on Linux and not any *nix. I don't have a strong opinion in the end; I think isLinux makes more sense than isNix or isOther. But the existing isWin / isMac are probably needed more often than isLinux, and it's easy to implement yourself.

@SethTisue
Copy link
Member

I'm inclined to merge this as-is, then.

SethTisue added a commit that referenced this pull request Mar 1, 2016
Added Properties.isLinux to compliment the isWin and isMac methods
@SethTisue SethTisue merged commit 662063d into scala:2.12.x Mar 1, 2016
@SethTisue
Copy link
Member

thank you Michael!

@Shadow53
Copy link
Contributor Author

Shadow53 commented Mar 7, 2016

No problem! Thanks for merging!

@adriaanm adriaanm added the welcome hello new contributor! label Apr 4, 2016
@adriaanm adriaanm added 2.12 and removed 2.12 labels Oct 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
welcome hello new contributor!
Projects
None yet
7 participants