-
Notifications
You must be signed in to change notification settings - Fork 3
Address !67 - Add support for queries in unit values #68
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
Conversation
| // $TODO Use of static is ugly. | ||
| // But there is no lazy initialization in this library. | ||
| // One can stuff this into a final's field assignment, but | ||
| // assigning final fields from methods is evil. Moving to Kotlin, | ||
| // would be "better" alternatives. | ||
|
|
||
| // the default implementation is good for units provided by this library, | ||
| // because it either x1000 or x1024, where the division always yields a | ||
| // regular fraction. If any unit that has an exuberant BPU is introduced, | ||
| // they should override this method. |
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.
since you are complaining here that we have no lazy init, how about we remove this entire method and hard-code the result of it in each conversionScale method of each unit? Since the result only depends on the number of bytes in each unit, this won't ever change for a unit anyway, so no need to compute it at runtime?
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 thought about it, and kinda decided to go with "let the computer do the calculations", to avoid it done menially, plus this method is still needed for tests to ensure that the scale is correct. If there was a lazy init, there would be a better way of handling it, avoiding a static variable in each implementation.
If you insist this was rather pre-computed, I can make it so.
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 was just doing lots of rust lately and kinda like this idea of running part of your code at compile time 😅 so I'm fine with the way it is and just wanted to provide an alternative solution
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 missed one thing here. I wasn't as much as "complaining that the lib has no lazy init" but rather stating so, and this might have sounded like a complaint, but about the fact that lazy init (or the lack thereof) in Java is a nuisance. One would have to drag lombok in, have its own solution made, or use dangerous class initialization patterns (which doesn't even offer the lazy part), neither of which should be necessary for the ask.
"Run your code at compile time" or "compute things easily during build time with arbitrary code" is a lot easier with Gradle (you can with Maven, but you have to have a plugin module which is again adding a lot more complexity and chicken-egg problems to the build), but I honestly had a lot more negative feedback for Gradle about how brittle the build becomes rather then praise to its offering devs to do "whatever" during it.
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.
yep all good here! I'll keep the comment around as-is, maybe a future version of Java will offer something new in this field 🤷
|
Code style is a little bit different from the rest of the repo, but I don't have any style checker enabled here, so LGTM on these changes since all tests are currently passing. Just out of curiosity: Are you implementing your own unit types? Can you share what these units are? |
Great. Would you be able to provide any ETA on a new version release with this?
No, not really. I really wanted to just get a unit value with arbitrary rounding. Given the hurdles that gave me trying to do this outside of the library, I figured let me take a stab at a PR, at which point I had to consider cases when somebody would implement their units for some inexplicable (to me) reason. |
|
Ok thanks again for answering my questions and putting in the work here! Highly appreciated ❤️ Wrt. next release: This should be running completely automated next wednesday, but it's been a while so I might have to do some fixes for the release pipeline first. I'll ping you once it's released! |
This adds the following into a storage unit:
I've changed the visibility of getNumberOfBytesPerUnit to
public, as I don't see a reason of hiding this. For abstract processing of unit objects in code, this can be vital to know.Tests added. Mvn verify passes.
I also altered the top-level pom.xml to specify the Java version that the compiler plugin must abide by.
I honestly did this so that IDEA picks up the right expectations on the source/target code level, which didn't work because it (IDEA) doesn't seem to know to run the property-populating plugin, but I still think it should be there, so the language level doesn't depend on the version of the JDK the code is compiled with.