Simplify CoordinateNumber.gerber() #9
Conversation
| // Format invariants | ||
| if format.decimal > DECIMAL_PLACES_CHARS { | ||
| return Err(GerberError::CoordinateFormatError("Invalid precision: Too high!".into())) | ||
| fn rounded_div(dividend: i64, divisor: i64) -> i64 { |
There was a problem hiding this comment.
Not sure if we should move this method to another location... It would be great to have this functionality in the Rust standard library :)
There was a problem hiding this comment.
We could ask in IRC or on Reddit whether something like that exists in the stdlib, or could possibly be added.
For now, you could create a utils module:
- Add a
utils.rsfile and put the function in there (with thepubmodifier) - Add a
mod utilsstatement tosrc/lib.rs - Add
use utils::rounded_divto the top of this file
| // Format invariants | ||
| if format.decimal > DECIMAL_PLACES_CHARS { | ||
| return Err(GerberError::CoordinateFormatError("Invalid precision: Too high!".into())) | ||
| fn rounded_div(dividend: i64, divisor: i64) -> i64 { |
There was a problem hiding this comment.
We could ask in IRC or on Reddit whether something like that exists in the stdlib, or could possibly be added.
For now, you could create a utils module:
- Add a
utils.rsfile and put the function in there (with thepubmodifier) - Add a
mod utilsstatement tosrc/lib.rs - Add
use utils::rounded_divto the top of this file
|
|
||
| // Convert to string | ||
| Ok(format!("{}{:0<width$}", integer, decimal, width=format.decimal as usize)) | ||
| let divisor: i64 = 10_i64.pow((DECIMAL_PLACES_CHARS - format.decimal) as u32); |
There was a problem hiding this comment.
I'm not actually sure if we need the i64 suffix at all. Could you try without it (also on line 103)? Maybe it can be derived.
| let integer: i64 = self.nano / DECIMAL_PLACES_FACTOR; | ||
| if integer > 10i64.pow(format.integer as u32) { | ||
| return Err(GerberError::CoordinateFormatError("Decimal is too large for chosen format".into())); | ||
| if self.nano >= 10_i64.pow((format.integer + DECIMAL_PLACES_CHARS) as u32) { |
There was a problem hiding this comment.
Negatives aren't supported here, right? We probably need self.nano.abs()?
There was a problem hiding this comment.
Good point! :) Fixed it and also added a test which covers this issue...
Also added a test which covers this issue.
| let divisor: i64 = 10_i64.pow((DECIMAL_PLACES_CHARS - format.decimal) as u32); | ||
| let number: i64 = rounded_div(self.nano, divisor); | ||
| let divisor = 10_i64.pow((DECIMAL_PLACES_CHARS - format.decimal) as u32); | ||
| let number = rounded_div(self.nano, divisor); |
There was a problem hiding this comment.
Actually I meant it the other way around. Doesn't this work?
let divisor: i64 = 10.pow((DECIMAL_PLACES_CHARS - format.decimal) as u32);
let number: i64 = rounded_div(self.nano, divisor);
Then it's still clear and explicit what the types are, but unnecessary literal suffixes aren't there. (But again, I'm not 100% sure if the type inference works properly here.)
There was a problem hiding this comment.
I have tried it, but it didn't work :(
There was a problem hiding this comment.
Ok, then I'd simply undo commit 025494a (git reset --hard HEAD^ && git push --force).
ubruhin
force-pushed
the
simplify_coordinatenumber
branch
from
025494a
to
7f510d5
Compare
|
Now it's perfect :) |
I think this makes the function
CoordinateNumber.gerber()more robust and easier to understand.