4 ms·

The root cause here is poorly named settings. If the original setting had been named something bool-y like `help.autocorrect_enabled`, then the request to acce

The root cause here is poorly named settings. If the original setting had been named something bool-y like `help.autocorrect_enabled`, then the request to accept an int (deciseconds) would've made no sense. Another setting `help.autocorrect_accept_after_dsec` would've been required. And `dsec` is so oddball that anyone who uses it would've had to look up. I insist on this all the time in code reviews. Variables must have units in their names if there's any ambiguity. For example, `int timeout` becomes `int timeout_msec`. This is 100x more important when naming settings, because they're part of your public interface and you can't ever change them.

yencabulator 2y ago
I do that, but I can't help thinking that it smells like Hungarian notation. The best alternative I've found is to accept units in the values, "5 seconds" or "5s". Then just "1" is an incorrect value.
- physicles 2y ago
  That’s not automatically bad. There are two kinds of Hungarian notation: systems Hungarian, which duplicates information that the type system should be tracking; and apps Hungarian, which encodes information you’d express in types if your language’s type system were expressive enough. [1] goes into the difference. [1] https://www.joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong/ https://www.joelonsoftware.com/2005/05/11/making-wrong-code-...
  - yencabulator 2y ago
    And this is exactly the kind the language should have a type for, Duration.
    - crazygringo 2y ago
      Not really. I don't want to have a type for an integer in seconds, a type for an integer in minutes, a type for an integer in days, and so forth. Just like I don't want to have a type for a float that means width, and another type for a float that means height. Putting the unit (as oppose to the data type) in the variable name is helpful, and is not the same as types. For really complicated stuff like dates, sure make a type or a class. But for basic dimensional values, that's going way overboard.
      - yencabulator 2y ago
        > I don't want to have a type for an integer in seconds, a type for an integer in minutes, a type for an integer in days, and so forth. This is not how a typical Duration type works. https://pkg.go.dev/time#Duration https://pkg.go.dev/time#Duration https://doc.rust-lang.org/nightly/core/time/struct.Duration.html https://doc.rust-lang.org/nightly/core/time/struct.Duration.... https://docs.rs/jiff/latest/jiff/struct.SignedDuration.html https://docs.rs/jiff/latest/jiff/struct.SignedDuration.html
        crazygringo 2y ago
        I'm just saying, this form of "Hungarian" variable names is useful, to always include the unit. Not everything should be a type. If all you're doing is calculating the difference between two calls to time(), it can be much more straightforward to call something "elapsed_s" or "elapsed_ms" instead of going to all the trouble of a Duration type.
      - thaumasiotes 2y ago
        > I don't want to have a type for an integer in seconds, a type for an integer in minutes, a type for an integer in days, and so forth. > For really complicated stuff like dates, sure make a type or a class. Pick one. How are you separating days from dates? Not all days have the same number of seconds.
      - ghusbands 2y ago
        You're missing the value of these things in identifying bugs. When you subtract a number of seconds from a temperature, you'll be glad for the compile-time error. There also doesn't have to be a runtime cost, as in C++ (and languages supporting value type instances) they can either be type-erased or cost no more than an integer.
TeMPOraL 2y ago
> I insist on this all the time in code reviews. Variables must have units in their names if there's any ambiguity. For example, `int timeout` becomes `int timeout_msec`. Same here. I'm still torn when this gets pushed into the type system, but my general rule of thumb in C++ context is: void FooBar(std::chrono::milliseconds timeout); is OK, because that's a function signature and you'll see the type when you're looking at it, but with variables, `timeout` is not OK, as 99% of the time you'll see it used like: auto timeout = gl_timeout; // or GetTimeoutFromSomewhere(). FooBar(timeout); Common use of `auto` in C++ makes it a PITA to trace down exact type when it matters. (Yes, I use IDE or a language-server-enabled editor when working with C++, and no, I don't have time to stop every 5 seconds to hover my mouse over random symbols to reveal their types.)
- physicles 2y ago
  Right, your type system can quickly become unwieldy if you try to create a new type for every slight semantic difference. I feel like Go strikes a good balance here with the time.Duration type, which I use wherever I can (my _msec example came from C). Go doesn’t allow implicit conversion between types defined with a typedef, so your code ends up being very explicit about what’s going on.
- theamk 2y ago
  It should not matter though, because std::chrono is not int-convertible - so is it "milliseconds" or "microseconds" or whatever is an minor implementation detail. You cannot compile FooBar(5000), so there is never confusion in C++ like C has. You have to do explicit "FooBar(std::chrono::milliseconds(500))" or "FooBar(500ms)" if you have literals enabled. And this will handle conversion if needed - you can always do FooBar(500ms) and it will work even if actual type in microseconds. Similarly, your "auto" example will only compile if gl_timeout is a compatible type, so you don't have to worry about units at all when all your intervals are using std::chrono.
- OskarS 2y ago
  One of my favorite features of std::chrono (which can be a pain to use, but this part is pretty sweet) is that you don't have to specify the exact time unit, just a generic duration. So, combined with chrono literals, both of these work just like expected: std::this_thread::sleep_for(10ms); // sleep for 10 milliseconds std::this_thread::sleep_for(1s); // sleep for one second std::this_thread::sleep_for(50); // does not work, unit is required by type system That's such a cool way to do it: instead of forcing you to specify the exact unit in the signature (milliseconds or seconds), you just say that it's a time duration of some kind, and let the user of the API pick the unit. Very neat!
bmicraft 2y ago
> Variables must have units in their names if there's any ambiguity Then you end up with something where you can write "TimoutSec=60" as well as "TimeoutSec=1min" in the case of systemd :) I'd argue they'd been better of not putting the unit there. But yes, aside from that particular weirdness I fully agree.
- physicles 2y ago
  > Then you end up with something where you can write "TimoutSec=60" as well as "TimeoutSec=1min" in the case of systemd :) But that's wrong too! If TimeoutSec is an integer, then don't accept "1min". If it's some sort of duration type, then don't call it TimeoutSec -- call it Timeout, and don't accept the value "60".
  - whycome 2y ago
    Can we call this the microwave paradox
    - bmicraft 2y ago
      What's the connection here to microwaves? I don't get it.
bambax 2y ago
Yes! As it is, '1' is ambiguous, as it can mean "True" or '1 decisecond', and deciseconds are not a common time division. The units commonly used are either seconds or milliseconds. Using uncommon units should have a very strong justification.
MrDresden 2y ago
> I insist on this all the time in code reviews. Variables must have units in their names if there's any ambiguity. For example, `int timeout` becomes `int timeout_msec`. Personally I flag any such use of int in code reviews, and instead recommend using value classes to properly convey the unit (think Second(2) or Millisecond(2000)). This of course depends on the language, it's capabilities and norms.
- kqr 2y ago
  I agree. Any time we start annotating type information in the variable name is a missed opportunity to actually use the type system for this. I suppose this is the "actual" problem with the git setting, in so far as there is an "actual" problem: the variable started out as a boolean, but then quietly turned into a timespan type without triggering warnings on user configs that got reinterpreted as an effect of that.
scott_w 2y ago
Yes and it's made worse by using "deciseconds," a unit of time I've used literally 0 times in my entire life. If you see a message saying "I'll execute in 1ms," you'd look straight to your settings!
miohtama 2y ago
It's almost like Git is a version control system built by developers who only knew Perl and C.
deltaburnt 2y ago
Though, ironically, msec is still ambiguous because that could be milli or micro. It's often milli so I wouldn't fault it, but we use micros just enough at my workplace where the distinction matters. I would usually do timeout_micros or timeout_millis.
- thousand_nights 2y ago
  can also do usec for micro
- hnuser123456 2y ago
  Shouldn't that be named "usec"? But then again, I can absolutely see someone typing msec to represent microseconds.
- seszett 2y ago
  We use "ms" because it's the standard SI symbol. Microseconds would be "us" to avoid the µ. In fact, our French keyboards do have a "µ" key (as far as I remember, it was done so as to be able to easily write all SI prefixes) but using non-ASCII symbols is always a bit risky.
- 3eb7988a1663 2y ago
  ms for microseconds would be a paddlin'. The micro prefix is μ, but a "u" is sufficient for easy of typing on an ascii alphabet.
jayd16 2y ago
What would you call the current setting that takes both string enums and deciseconds?
- physicles 2y ago
  help.autocorrect_enabled_or_accept_after_dsec? A name scary enough to convince anyone who uses it to read the docs.

yencabulator 2y ago

I do that, but I can't help thinking that it smells like Hungarian notation. The best alternative I've found is to accept units in the values, "5 seconds" or "5s". Then just "1" is an incorrect value.

physicles 2y ago
That’s not automatically bad. There are two kinds of Hungarian notation: systems Hungarian, which duplicates information that the type system should be tracking; and apps Hungarian, which encodes information you’d express in types if your language’s type system were expressive enough. [1] goes into the difference. [1] https://www.joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong/ https://www.joelonsoftware.com/2005/05/11/making-wrong-code-...
- yencabulator 2y ago
  And this is exactly the kind the language should have a type for, Duration.
  - crazygringo 2y ago
    Not really. I don't want to have a type for an integer in seconds, a type for an integer in minutes, a type for an integer in days, and so forth. Just like I don't want to have a type for a float that means width, and another type for a float that means height. Putting the unit (as oppose to the data type) in the variable name is helpful, and is not the same as types. For really complicated stuff like dates, sure make a type or a class. But for basic dimensional values, that's going way overboard.
    - yencabulator 2y ago
      > I don't want to have a type for an integer in seconds, a type for an integer in minutes, a type for an integer in days, and so forth. This is not how a typical Duration type works. https://pkg.go.dev/time#Duration https://pkg.go.dev/time#Duration https://doc.rust-lang.org/nightly/core/time/struct.Duration.html https://doc.rust-lang.org/nightly/core/time/struct.Duration.... https://docs.rs/jiff/latest/jiff/struct.SignedDuration.html https://docs.rs/jiff/latest/jiff/struct.SignedDuration.html
      - crazygringo 2y ago
        I'm just saying, this form of "Hungarian" variable names is useful, to always include the unit. Not everything should be a type. If all you're doing is calculating the difference between two calls to time(), it can be much more straightforward to call something "elapsed_s" or "elapsed_ms" instead of going to all the trouble of a Duration type.
    - thaumasiotes 2y ago
      > I don't want to have a type for an integer in seconds, a type for an integer in minutes, a type for an integer in days, and so forth. > For really complicated stuff like dates, sure make a type or a class. Pick one. How are you separating days from dates? Not all days have the same number of seconds.
    - ghusbands 2y ago
      You're missing the value of these things in identifying bugs. When you subtract a number of seconds from a temperature, you'll be glad for the compile-time error. There also doesn't have to be a runtime cost, as in C++ (and languages supporting value type instances) they can either be type-erased or cost no more than an integer.

TeMPOraL 2y ago

> I insist on this all the time in code reviews. Variables must have units in their names if there's any ambiguity. For example, `int timeout` becomes `int timeout_msec`. Same here. I'm still torn when this gets pushed into the type system, but my general rule of thumb in C++ context is: void FooBar(std::chrono::milliseconds timeout); is OK, because that's a function signature and you'll see the type when you're looking at it, but with variables, `timeout` is not OK, as 99% of the time you'll see it used like: auto timeout = gl_timeout; // or GetTimeoutFromSomewhere(). FooBar(timeout); Common use of `auto` in C++ makes it a PITA to trace down exact type when it matters. (Yes, I use IDE or a language-server-enabled editor when working with C++, and no, I don't have time to stop every 5 seconds to hover my mouse over random symbols to reveal their types.)

physicles 2y ago
Right, your type system can quickly become unwieldy if you try to create a new type for every slight semantic difference. I feel like Go strikes a good balance here with the time.Duration type, which I use wherever I can (my _msec example came from C). Go doesn’t allow implicit conversion between types defined with a typedef, so your code ends up being very explicit about what’s going on.
theamk 2y ago
It should not matter though, because std::chrono is not int-convertible - so is it "milliseconds" or "microseconds" or whatever is an minor implementation detail. You cannot compile FooBar(5000), so there is never confusion in C++ like C has. You have to do explicit "FooBar(std::chrono::milliseconds(500))" or "FooBar(500ms)" if you have literals enabled. And this will handle conversion if needed - you can always do FooBar(500ms) and it will work even if actual type in microseconds. Similarly, your "auto" example will only compile if gl_timeout is a compatible type, so you don't have to worry about units at all when all your intervals are using std::chrono.
OskarS 2y ago
One of my favorite features of std::chrono (which can be a pain to use, but this part is pretty sweet) is that you don't have to specify the exact time unit, just a generic duration. So, combined with chrono literals, both of these work just like expected: std::this_thread::sleep_for(10ms); // sleep for 10 milliseconds std::this_thread::sleep_for(1s); // sleep for one second std::this_thread::sleep_for(50); // does not work, unit is required by type system That's such a cool way to do it: instead of forcing you to specify the exact unit in the signature (milliseconds or seconds), you just say that it's a time duration of some kind, and let the user of the API pick the unit. Very neat!

bmicraft 2y ago

> Variables must have units in their names if there's any ambiguity Then you end up with something where you can write "TimoutSec=60" as well as "TimeoutSec=1min" in the case of systemd :) I'd argue they'd been better of not putting the unit there. But yes, aside from that particular weirdness I fully agree.

physicles 2y ago
> Then you end up with something where you can write "TimoutSec=60" as well as "TimeoutSec=1min" in the case of systemd :) But that's wrong too! If TimeoutSec is an integer, then don't accept "1min". If it's some sort of duration type, then don't call it TimeoutSec -- call it Timeout, and don't accept the value "60".
- whycome 2y ago
  Can we call this the microwave paradox
  - bmicraft 2y ago
    What's the connection here to microwaves? I don't get it.

bambax 2y ago

Yes! As it is, '1' is ambiguous, as it can mean "True" or '1 decisecond', and deciseconds are not a common time division. The units commonly used are either seconds or milliseconds. Using uncommon units should have a very strong justification.

MrDresden 2y ago

> I insist on this all the time in code reviews. Variables must have units in their names if there's any ambiguity. For example, `int timeout` becomes `int timeout_msec`. Personally I flag any such use of int in code reviews, and instead recommend using value classes to properly convey the unit (think Second(2) or Millisecond(2000)). This of course depends on the language, it's capabilities and norms.

kqr 2y ago
I agree. Any time we start annotating type information in the variable name is a missed opportunity to actually use the type system for this. I suppose this is the "actual" problem with the git setting, in so far as there is an "actual" problem: the variable started out as a boolean, but then quietly turned into a timespan type without triggering warnings on user configs that got reinterpreted as an effect of that.

scott_w 2y ago

Yes and it's made worse by using "deciseconds," a unit of time I've used literally 0 times in my entire life. If you see a message saying "I'll execute in 1ms," you'd look straight to your settings!

miohtama 2y ago

It's almost like Git is a version control system built by developers who only knew Perl and C.

deltaburnt 2y ago

Though, ironically, msec is still ambiguous because that could be milli or micro. It's often milli so I wouldn't fault it, but we use micros just enough at my workplace where the distinction matters. I would usually do timeout_micros or timeout_millis.

thousand_nights 2y ago
can also do usec for micro
hnuser123456 2y ago
Shouldn't that be named "usec"? But then again, I can absolutely see someone typing msec to represent microseconds.
seszett 2y ago
We use "ms" because it's the standard SI symbol. Microseconds would be "us" to avoid the µ. In fact, our French keyboards do have a "µ" key (as far as I remember, it was done so as to be able to easily write all SI prefixes) but using non-ASCII symbols is always a bit risky.
3eb7988a1663 2y ago
ms for microseconds would be a paddlin'. The micro prefix is μ, but a "u" is sufficient for easy of typing on an ascii alphabet.

jayd16 2y ago

What would you call the current setting that takes both string enums and deciseconds?

physicles 2y ago
help.autocorrect_enabled_or_accept_after_dsec? A name scary enough to convince anyone who uses it to read the docs.