Add isAtLeast(OS) method #5
Conversation
| */ | ||
| public boolean isAtLeast(OS operatingSystem) { | ||
| // Restrict to families with versions | ||
| enforceFamily(Family.MAC, Family.WINDOWS); |
There was a problem hiding this comment.
I realized later perhaps we can allow other OS's to equal each other if the family matches so this should probably be changed to enforceNotFamily(Family.UNKNOWN)
There was a problem hiding this comment.
Even in the case of the family being unknown the OSs could still equal each other though, right? As I explained in my unfortunately timed comment below I am generally not a fan of the use of enforce here anyways, but more on that in the comment! ;)
There was a problem hiding this comment.
The problem here is that "other" is a catch all for any other OS that doesn't parse/isn't implemented. So you could have FreeBSD equal to OpenBSD, for example, which is not the case.
| assertFalse(OS.MAC_OSX_LION.isAtLeast(OS.MAC_OSX_MOUNTAIN_LION), message); | ||
|
|
||
| String notThrowMessage = "Did not throw even though OS was not allowed!"; | ||
| assertThrows(UnsupportedOSException.class, () -> OS.UBUNTU.isAtLeast(OS.UBUNTU), |
There was a problem hiding this comment.
This should maybe be changed to be "true".
There was a problem hiding this comment.
This is a great idea, thanks a lot!
One thing I'd like to change is the fact that you have those enforce calls inside isAtLeast. I can understand why one would want to throw an exception in those cases but in my opinion that is overkill in this scenario and just returning false would be enough. I can see this method being used instead of OS.isFamily and we wouldn't want that throwing an exception if the result was false.
Also, based on the other helper methods the user doesn't expect one starting with "is" to throw an exception. That's what "enforce" is for. For this reason I'd actually like to keep the code you wrote, but move it to a separate function public void enforceAtLeast(OS operatingSystem) and have it throw an exception if the result of compareTo is smaller than 0.
|
So two methods; the |
Yes, exactly! :) |
|
About this: I'm not sure if you missed my last comment...
...but that's the reason I haven't merged this yet. I'd have no problem merging and then implementing this little fix myself, but before I do that I wanted to make sure I don't go over your head, in case you do actually want to do this yourself or disagree. |
|
I think I got confused by UNKNOWN (OS) vs. OTHER (Family). Really the correct solution is to put SOLARIS in a UNIX family along with AIX (which currently matches into LINUX). Although you can change that conditional to just an UNKNOWN OS. I'm fine if you want to tweak the logic after you merge (or even push to the PR fork before you merge... if you haven't learned how to do that as a maintainer, you should! ) |
Yep, I'll implement that soon.
Just did that. I really am trying my best to do a good job of this, even though I'm new to all of it, believe me. I look forward to learning a lot more as this project progresses, haha. |
It's really one of the best places out there to self-learn software engineering! Try getting hired as a senior software engineer based primarily on 7 years of maintaining an open source project. ;) |
|
Haha, sounds like a good plan, see you in 7 years then ;) |
Fixes #3
Makes the assumption the "Unknown" versions are newer, which might not be true but is more likely to be true than not.
Used your same formatting with tabs, which are evil. ;)