Add support for min version #28
Conversation
|
Bump. Would it be possible for someone to take a look and review this? |
src/lib.rs
Outdated
| let ver: Vec<u8> = version | ||
| .split(" ") | ||
| .skip(2) | ||
| .next() | ||
| .map(|ver| { | ||
| ver.split(".") | ||
| .map(|v| v.parse().expect("Invalid version component")) | ||
| .collect() | ||
| }) | ||
| .expect("Invalid version"); |
There was a problem hiding this comment.
There has to be a better way, I have no idea looking at this what this parsing does. My feeling is that some sort of regex may be a better solution.
There was a problem hiding this comment.
the regex is big, we can use it, but isn't exactly optimal.
src/lib.rs
Outdated
| } else { | ||
| Ok(()) | ||
| } | ||
| } | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } | ||
| Err(err) => Err(err)?, |
There was a problem hiding this comment.
All this can probably be unrolled using the ? operator, rather than all these nested if statements. Several test cases, each of which returns Err if it fails.
src/lib.rs
Outdated
| @@ -180,6 +182,12 @@ impl Build { | |||
| self | |||
| } | |||
|
|
|||
| /// Set the minimum version required | |||
| pub fn min_version(&mut self, major: u8, minor: u8, micro: u8) -> &mut Self { | |||
There was a problem hiding this comment.
Why u8 here? I feel like it can only cause problems to not just use usize in this case
There was a problem hiding this comment.
sure, it can be changed that way.
|
Made a bit simpler, I did not use regex to keep nasm-rs as lean as possible. |
| } else { | ||
| if major > ver[0] || | ||
| (major == ver[0] && minor > ver[1]) || | ||
| (major == ver[0] && minor == ver[1] && micro > ver[2]) { | ||
| Err(version)? | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Since the previous if statement panic!s, this code doesn't need an explicit else branch,
No description provided.