sindresorhus / eslint-plugin-unicorn

More than 100 powerful ESLint rules
MIT License
4.25k stars 367 forks source link

`expiring-todo-comments` Unintuitiveness with partial version numbers #1132

Open Tenga opened 3 years ago

Tenga commented 3 years ago

Hi maintainers. 👋

First off, thanks for developing and maintaining this package, it's been really useful!

However, I have a minor unintuitive nitpick with how the expiring-todo-comments rule handles partial versions.

The problem

To preface:

However, I believe, there is a minor unintuitive case here where versions that don't specify all three positions.

When reading/writing the TODO comments and encountering or writing >4, my brain goes

Anything within v4.x.x will pass lint and this TODO won't trigger lint until we upgrade the major version past v4.x.x

and same goes for >4.1 where it goes

Anything within v4.1.x will pass lint and this TODO won't trigger lint until we upgrade the major past v4.x.x or minor version of v4 past v4.1.x

However, what this really means is:

This can be avoided by suppressing my own monkey brain:

One might argue that even the documentation gets this wrong. It's a bit hard to assume the intent here due to missing context but I believe I can assume the context, so I am using them to support my assertion of unintuitiveness by showing the problems the examples have.

The examples that are currently here state:

// TODO (@lubien) [>0]: For now this is fine but for a stable version we must refactor.
// FIXME [>10]: This feature is deprecated and should be removed from the next major version.

First example will always trigger at any version other than 0.0.0.

The second example will trigger on 10.0.1 and higher, while the intent seems to be to say >=11.

Of course, there are other examples that reason about this correctly, but these two seemingly don't, and it's understandable why.

Potential proposed resolutions

Do nothing

I mean, nothing is really wrong here, it works as underlying logic would dictate. It's just a bit unintuitive, and when writing such comments, I shouldn't really assume 4 expands to a range 4.x.x but to version 4.0.0 specifically.

Readme examples might need correcting, if my assumptions on them are correct.

Presume that missing minor/patch places imply a range

This would fix the assumed unintuitiveness in this specific case, but would probably be a breaking change to the rule.

Even the support for explicitly setting X-range (1.x.x, 1.1.x) version seems like it would be a breaking change as they also currently get resolved in the same manner. The lack of intuitiveness here would remain unless the support for partial versions is also removed to prevent it.


I don't believe this is quite a bug or a new rule, but I believe it's closer to a bug, so I've created it as that.

Also, apologies for any premature issue notifications, opened the issue prematurely by accident. 😓

fisker commented 3 years ago

How about use >=5

Tenga commented 3 years ago

How about use >=5

Hi! Thanks for the response.

As I mention in the issue itself, I'm aware that to get the intended result of the intent "when major version is higher than 4" is to write >=5.

What I am trying to raise is the following:

If there was a poll to ask the following two questions:

I am currently under the impression that the answers to that poll of those who answer instinctively, without checking, might be:

And those would not be the correct answers as it currently stands. Granted, my impression might be wrong, as I'm basing it off of asking those questions to a couple of people, and I've found it that the answers are the ones above, but I felt like it was worthwhile to raise it as a potential issue.

It's an minor annoyance, that I feel the people using expiring-todo-comments can fall into, due to instinct as to what a partial version number means.

I'm just highlighting the potential to maybe address this potential "instinct issue" at some point, if the maintainers agree with my assertation above.

No hard feelings if we don't agree, and am open to have my mind changed on that opinion.