deprecation(semver): rename testRange() to satisfies()

[timreichen]

Changes

• Deprecates testRange() in favour of satisfies().
• The arguments are switched around to follow the naming scheme. Range satisfies version: satisfies(range: Range, version: Version)
• All instances of testRange() used in std/semver are replaced with satisfies().

Reasons

• The name testRange has the focus on range rather on the version, to check if it is part of the range.
• The name testRange can be confused for range validation.
• test at the end of a file name has special meaning to deno in that the file will run tests with deno test. having test as part of the function makes file names less comprehensive (test_range.ts and test_range_test.ts).

Derivations
- The name includes is borrowed from Array.prototype.includes(), which checks if an element is part of array. See #4364

[kt3k]

I'm not in favor of this change. The reasons don't seem giving enough reasoning to do the deprecation and breaking change.

#4090 (comment)

Javascript generally has a verb-first convention for functions.

Also the name rangeIncludes() conflicts with the above convention.

[timreichen]

#4090 (comment)

Javascript generally has a verb-first convention for functions.

Also the name rangeIncludes() conflicts with the above convention.

rangeX() schema was borrowed from rangeIntersects() wich is currently present in std/semver. On that note we probably should change that to intersectsRange() to be consistent.

I'm not in favor of this change. The reasons don't seem giving enough reasoning to do the deprecation and breaking change.

I think testRange() is a misnomer. The function doesn't test a range as the name implies, but checks if a version is in range.
So if maybe inRange() instead? One would read it as (is) version in range?

[iuioiua]

I agree with the ideas in this PR for the reasons Tim gives. rangeIncludes() is far clearer in stating the purpose of the function. I'd only add that rangeIncludes() might make more sense if it represented a hypothetical range.includes(), which makes sense to me.

[timreichen]

inRange(version: SemVer) would also be a possibility.

[timreichen]

inRange(version: SemVer) would also be a possibility.

@kt3k WDYT?

[kt3k]

We need strong reasons for doing breaking changes, such as resolving inconsistency, fixing obvious design errors, etc. 'Some name looking better' can't be a reason for breaking change in my view.

[iuioiua]

Hey Yoshiya, I agree with Tim. However, we've not fully articulated our reasoning.

The purpose of testRange() isn't clear. It actually raises more questions than it answers. For example, when I first read the function name, I thought,

• Is it testing some aspect of Range? If so, what aspect is it?
• Is it testing that an argument is a Range?

Once one learns the function's purpose, one may be surprised to know they were asking the wrong questions in the first place due to the misleading name.

However, the purpose of the function is far clearer when talking about SemVers than something like rangeIncludes() or inRange() or similar.

I think this is more than just a preference for function names; it is a decent improvement in the clarity of the function's objective and reduces the amount of reading of the documentation before use. I think the value gained is worth the breaking change.

[kt3k]

I think the name has been derived from RegExp.prototype.test which test a string with RegExp. Similarly testRange tests a semver with the given range. I don't think this is too misleading or confusing

[iuioiua]

Based on my and Tim's opinions and those of others expressed in Discord, the majority of users believe this function name is confusing. I think changing the name is a good idea.

[kt3k]

I'm still not convinced with the deprecation, but I see some issues with the suggested replacement.

include verb is not aligned with the names maxSatisfying and minSatisfying that use the verb satisfy for it.

Also the jsdoc of rangeIncludes says Test to see if the version satisfies the range. describing it with satisfy.

Another note: the original npm:semver module calls this API satisfies(version, range).

So the use of include verb looks random and unnecessary

[iuioiua]

Actually, satisfiesRange() might be a better name. What do we think?

[kt3k]

How about satisfies? A version can only satisfies 'ranges', and no other things can be satisfied by a version. So ~Range part doesn't have much effect.

Also it's the same name as npm:semver's, and the users would be more familiar with that name.

[iuioiua]

How about satisfies? A version can only satisfies 'ranges', and no other things can be satisfied by a version. So ~Range part doesn't have much effect.

Also it's the same name as npm:semver's, and the users would be more familiar with that name.

+1

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.93%. Comparing base (4a1ed50) to head (85d8a4e).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4364      +/-   ##
==========================================
- Coverage   90.99%   90.93%   -0.07%     
==========================================
  Files         478      479       +1     
  Lines       37383    37399      +16     
  Branches     5308     5310       +2     
==========================================
- Hits        34016    34008       -8     
- Misses       3305     3329      +24     
  Partials       62       62              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iuioiua]

@timreichen and @kt3k, I've modified this PR to introduce the new function satisfies() instead of rangeIncludes(). I'm happy with this name, and it is clearer than testRange(). WDYT?

[kt3k]

LGTM