Hacker News Reader

Comment by physicles | Hacker News Reader

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.

View on HN

> 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.)

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.

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.

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!

I do something similar in Java by taking a java.time.Duration in any method dealing with time. We don't have the snazzy literal syntax, but that means users have to write:

  someMethodDealingWithTime(Duration.ofMillis(10));
  someMethodDealingWithTime(Duration.ofSeconds(1));
  someMethodDealingWithTime(50); // does not compile

Since these often come from config, i also have a method parseDuration which accepts a variety of simple but unambiguous string formats for these, like "10ms", "1s", "2h30m", "1m100us", "0", "inf", etc. So in config we can write:

  galactus.requestTimeout=30s

No need to bake the unit into the name, but also less possibility of error.

> i also have a method parseDuration which accepts a variety of simple but unambiguous string formats for these, like "10ms", "1s", "2h30m", "1m100us", "0", "inf", etc.

I did that too with parsers for configuration files; my rule of thumb is that the unit has to always be visible somewhere anywhere a numeric parameter occurs - in the type, in the name, or in the value. Like e.g.:

  // in config file:
  { ..., "timeout": "10 seconds", ... }

  // in parsing code:
  auto& ParseTimeout(const std::string&) -> Expected<std::chrono::milliseconds>;

  // in a hypothetical intermediary if, for some reason, we need to use a standard numeric type:
  int timeoutMsec = ....;

Wrt. string formats, I usually allowed multiple variants for a given time unit, so e.g. all these were valid and equivalent values: "2h", "2 hour", "2 hours". I'm still not convinced it was the best idea, but the Ops team appreciated it.

(I didn't allow mixing time units like "2h30m" in your example, as to simplify parsing into single "read double, read rest as string key into a lookup table" pass, but I'll think about allowing it the next time I'm in such situations. Are there any well-known pros/cons to this?)

Mixed unit durations are in ISO 8601, so the idea has had at least some scrutiny:

https://en.wikipedia.org/wiki/ISO_8601#Durations

One place i have run into confusion is being able to express a given span of time in multiple ways. 1m30s and 90s are the same length, but are they the same thing? Should we always normalise? If we do, do we normalise upwards or downwards? This hasn't actually been a problem with time, but i do similar handling with dates, and it turns out we often want to preserve the distinction between 1y6m and 18m. But also sometimes don't. Fun times.

> Mixed unit durations are in ISO 8601

Don't know why I never noticed it before; thanks for posting this! That does give the idea more weight, so I'll consider mixed-unit durations next time I find myself coding up parsing durations in config files.

> Should we always normalise? If we do, do we normalise upwards or downwards?

I'd say normalize, but on the business side, down to regular units - e.g. the config or UI can keep its "1m30s" or "90s" or even "0.025h", but for processing, this gets casted to seconds or millis or whatever the base unit is. Now, this is easy when we're only reading, but if we need to e.g. modify or regenerate the config from current state, I'd be leaning towards keeping around the original format as well.

> i do similar handling with dates, and it turns out we often want to preserve the distinction between 1y6m and 18m

Can you share specific examples where does this matter, other than keeping the user input in the format it was supplied even underlying data values get regenerated from scratch?

> 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.

JetBrains does a great thing where they show types for a lot of things as labels all the time instead of having to hover over all the things.

Right; the so-called "inlay hints" are also provided by clangd over LSP, so I have them in my Emacs too. Super helpful, but not always there when I need them.

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!

> 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.

> 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".

Can we call this the microwave paradox

What's the connection here to microwaves? I don't get it.

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.

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-...

And this is exactly the kind the language should have a type for, Duration.

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.

> 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://doc.rust-lang.org/nightly/core/time/struct.Duration....

https://docs.rs/jiff/latest/jiff/struct.SignedDuration.html

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.

> 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.

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.

> 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.

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.

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.

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.

can also do usec for micro

Shouldn't that be named "usec"? But then again, I can absolutely see someone typing msec to represent microseconds.

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.

ms for microseconds would be a paddlin'. The micro prefix is μ, but a "u" is sufficient for easy of typing on an ascii alphabet.

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

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

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

Reply on news.ycombinator.com