unreadable_literal for floating-point numbers

[kornelski]

unreadable_literal triggers for floating-point numbers such as

  |
5 |     0.095332, 0.118095, 0.095332,
  |                         ^^^^^^^^ help: consider: `0.095_332`

To me this is weird, because I consider _ in numbers as an equivalent of a thousands separator, and I've never seen thousands separators used in the fractional part of a decimal fraction.

In integers, the thousands separators matter, because you can't see magnitude of the number without counting digits. But in the fractional part, the length doesn't matter: the most significant digits are always in the same place, easy to find.

I suggest not linting the fractional part, as it's weird and unnecessary.

---

clippy 0.0.212 (265318d 2019-05-17)

[flip1995]

the most significant digits are always in the same place, easy to find

Isn't that also the case for integer numbers? 😄

I think the reasoning behind this is, that 0.763459873694934 is way less readable, than 0.763_459_873_694_934. "Separating by thousands" is just consistency with the spacing of the integer part.

It's a style lint, so it is opinionated.

On course of action would be to separate this lint into the integer and the fractional part, so you can disable the fractional and integer part separately. But I would keep both lints warn-by-default.

[kornelski]

For integers the grouping adds clarity:

    1000000
   10000000
  100000000

  1_000_000
 10_000_000
100_000_000

and from the leftmost group you can easily see the difference between 1, 10, 100.

However, for fractions . already acts as the visual anchor:

0.1000000
0.10000000
0.100000000

and the grouping doesn't make any sense:

0.100_000_0
0.100_000_00
0.100_000_000

the rightmost 0, 00, 000 don't have any meaning. The false grouping amplifies an insignificant difference.

I have seen people group digits after the decimal. For example, in Wikipedia articles they group them with "narrow gaps" into groups of three or five (style guide). However, I feel they aren't very useful and add a lot of visual noise. I'd like to know if other people like them.

[flip1995]

the rightmost 0, 00, 000 don't have any meaning.

Ah thanks for the clarification! I agree with that. IMO the fractional part get's more readable when separated by _. And separating by 3 digits is consistent with the integer part.

I'd like to know if other people like them.

Yeah I like them, because they improve the readability, especially for long fractionals. Since this is obviously more opinionated than the integer part, I think separating these lints would be the best thing to do, so you can easily turn them on/off by it's own.

[xFrednet]

Hey, I would take this issue up if that's okay? :)

How should the lint be split up? We could simply have one for the integer and one for the fraction part like this:

Example:

//       Linted by the the integer lint
//       vvvvvvvvv
let _a = 100000000.010010010010
//                 ^^^^^^^^^^^^
//                 Linted by the fraction lint

The only drawback I see is that it would give two message for the example number because it would violate the integer and the fraction lint. This would probably give an output like:

error: long literal lacking separators
 --> src/main.rs:5:15
  |
5 |     let _a = 100000000.010010010010
  |              ^^^^^^^^^ help: consider: `100_000_000`
  |
error: long literal fraction lacking separators
 --> src/main.rs:5:15
  |
5 |     let _a = 100000000.010010010010
  |                        ^^^^^^^^^^^^ help: consider: `010_010_010_010`
  |

I can probably implement it like this if it looks good :)

[flip1995]

No that I'm over 1 year older and wiser, than at the time of my previous comment, I would not split this lint up into 2. Instead we should add a configuration option "lint_fractional" or a better name), which by default is true. People who don't like this behavior can then specify, that this lint should not lint on the fractional part and the problem you noticed @xFrednet goes away.

[xFrednet]

That sounds like a better plan 👍 . I'm also up to implement that option it that's okay? Could you maybe point me to a different lint that uses an option as a reference? 🙃 @flip1995

[flip1995]

Sure! Here's a PR, that added a configuration option: #5761

Some other things were done in this PR. Basically what you have to do is add the option in conf.rs, then give it to the lint struct in lib.rs and last but not least change the lint, so that it checks for this configuration. You can test configuration options by adding a test dir to tests/ui-toml.