Fix a bug when comparing ranged version to non-ranged version

[queengooborg]

This PR intends to fix a bug that was discovered during #5246, when a ranged version in version_added has the same number as a non-ranged version in version_removed, or vice versa. It was also found that because the linter just strips the ≤ when one is a ranged value, then if version_added was a ranged value and version_removed was a defined value within that range, the linter would fail.

This PR updates the logic so that a ranged value for version_added is equivalent to "0" (basically the lowest value), and a ranged value for version_removed is equivalent to whatever that version is. (Example: "version_added": "≤37", "version_removed": "≤37" translates to "version_added": "0", "version_removed": "37" for the linter.)

As an additional step to reduce the complexity of the code, this removes the special case when both values are ranged.

(Note: There is also #5501, which fixes the first bug (when version_added is a ranged value with the number equivalent to version_removed), however the author of that PR showed no interest in updating that PR to include the fix for the second bug found (version_removed is less than the version number of version_added). This fix, however, simplifies the code and makes the other PR's code completely redundant -- as such, this PR closes #5501.)

[queengooborg]

May I request a priority review on this, @Elchi3 / @ddbeck? We’ve had a PR blocked by this one, and the changes are small, so I’d love to get this in as soon as possible!

[Elchi3]

This logic requires some time to think it through. One thing I can say already: A comment explaining the motivation for '0' would be nice for anyone who looks at this in the future.

[queengooborg]

Certainly! I picked “0” as the value because it’s a number that will be lower than any possible browser version, so it won’t cause any issues with any browsers/engines we’re recording beta versions for, like NodeJS. I was initially thinking “1” to indicate “the first browser release”, which would catch an issue where a feature’s version_removed is set to the first browser release, but that’s probably an edge case (though I’d be happy to throw something together for it).

[Elchi3]

Sorry, I meant a code comment would be useful, but thanks again for explaining it here as well.

[queengooborg]

Oh, haha — yeah, sure thing, I’ll push a commit to add a comment in the code!

[queengooborg]

It seems that #5950 is also suffering from this bug now...

[Elchi3]

In #5950 this looked pretty funky to me, so I will need to think this through some more and if we should really allow this or not. (it was resolved there to not have ≤37 anymore)

[queengooborg]

I'm not entirely sure why we would want to disallow these two cases...they both seem like valid cases to me?

[ddbeck]

The problem with the linter is not currently blocking any PRs, right? The two mentioned in the description seem to have been merged with changes, but I wanted to make sure before editing.

[queengooborg]

Yeah, luckily the two PRs this issue was blocking could be adjusted (one just had wrong data, and the other, an exact version could be determined).

[queengooborg]

Ran into this problem again when working on #7542, specifically with a statement reading version_added: ≤37, version_removed: 4.4. In that PR, I set the version_added to true to mitigate the issue for now.

Edit: this came up two more times since, with basically the same data.

[foolip]

This blocked #8349 so getting this resolved would be great. Is it just a matter of reviewing the code?

[ddbeck]

OK, if this is blocking, I'll review this soon (probably not until after the new year though). Please feel free to @ me on this if I haven't taken action by January 7.

[queengooborg]

@ddbeck Ping!

[ddbeck]

That's a lot nicer. Thank you! 🎉