So the next and final commit should be a message at the top of the README saying the library is unmaintained and contains serious vulnerabilities and users are advised to move to alternative XYZ.
Would have to be F32, no?
I cannot think of any way to enforce "non-zero-ness" of the result without making it return an optional Result<NonZeroF32>, and at that point we are basically back to square one...
> I cannot think of any way to enforce "non-zero-ness" of the result without making it return an optional Result<NonZeroF32>, and at that point we are basically back to square one...
`NonZeroU32::checked_add(self, other: u32)` basically does this, although I'll note it returns an `Option` instead of a `Result` ( https://doc.rust-lang.org/std/num/type.NonZeroU32.html#metho... ), leaving you to `.map_err(...)` or otherwise handle the edge case to your heart's content. Niche, but occasionally what you want.
> `NonZeroU32::saturating_add(self, other: u32)` is able to return `NonZeroU32` though!
I was confused at first how that could work, but then I realized that of course, with _unsigned_ integers this works fine because you cannot add a negative number...
You'd still have to check for overflow, I imagine.
And there are other gotchas, for instance it seems natural to assume that NonZeroF32 * NonZeroF32 can return a NonZeroF32, but 1e-25 * 1e-25 = 0 because of underflow.
That is my charitable interpretation, but it's always one or two changes across a module that has hundreds, maybe thousands of lines of code. I'd expect an auto-formatter to be more obvious.
In any case, just looking over your own PR briefly before submitting it catches these quickly. The lack of attention to detail is the part I find more frustrating than the actual unnecessary format changes.
Why would you are about blank lines? Sounds like aborted attempts at a change to me. Then realizing you don’t need them. Seeing them in your PR, and figuring they don’t actually do anything to me.
Agree that "splitting for splittings' sake" (only to stay below an arbitrary line count) does indeed not make sense.
On the other hand I often see functions like you describe - something has to be executed step-by-step (and the functionality is only used there) - where I _whish_ it was split up into separate functions, so we could have meaningful tests for each step, not only for the "whole thing".
If I have a huge function, and I can peel parts off into sensible well-encapsulated sub-functions, and I name them well, then my ability to comprehend the whole goes up.
If I do that, future me will thank me, because I will almost inevitably be back to that function at some point.
But for this to work, the sub-functions have to really do what they say, and not do anything more. I have to be able to trust that I can understand them by just their name and arguments.
I ran into some code recently where this pattern caused me so much headache - class A has an attribute which is an instance of class B, and class B has a "parent" attribute (which points to the instance of class A that class B is an attribute of):
class Foo:
def __init__(self, bar):
self.bar = bar
class Bar:
def __init__(self, foo):
self.foo = foo
Obviously both called into each other to do $THINGS... Pure madness.
So my suggestion: Try not to have interdependent classes :D
Well, at times having a parent pointer is rather useful! E.g. a callback registration will be able to unregister itself from everywhere where it has been registered to, upon request. (One would want to use weak references in this case.)
On the other hand, I tend to take it as a hint that I should look at my module structure, and see if I can avoid the cyclic import (even if before adding type hints there was no error, there still already was a "semantic dependency"...)
