Handle '', '*', '1.x.', 'URL#semver:...', etc. #1

jacobq · 2018-03-12T20:19:11Z

This PR makes the following changes:

Correctly detect ranges specified with ".x" notation, e.g. 1.0.x
Correctly detect ranges specified with *, '', or a.b.c - d.e.f notation
Improve test coverage, e.g. commitish URLs with exact (e.g. #hash) and range (e.g. #semver:~1.0.0)

(see https://docs.npmjs.com/files/package.json#dependencies)

bendrucker · 2018-03-12T21:45:44Z

Hi, I just updated standard. Tape did not need updating, your change was a noop and just forced a newer version that was previously allowed. Please don't make any changes to the package.

Updating necessary deps is ok but bumping the version or making other changes is poor PR etiquette.

I will try to clean this up and merge later this afternoon.

jacobq · 2018-03-12T22:12:04Z

@bendrucker Thanks for your reply. Per your request I've reverted the package.json changes except for bumping standard from ^4.0.0 to ^11.0.0. I wasn't trying to have bad PR etiquette; I've just found that often the authors/maintainers of projects that are this old/inactive do not want to invest any effort in them, so I was trying to make things easy. I appreciate you being willing to merge & publish an updated version.

bendrucker · 2018-03-12T22:24:41Z

That's ok, I appreciate the contribution. Trying to make things effort-free is nice but part of that is minimizing the change set.

The changes to the source are more substantial than expected so I'll leave some comments and we'll work on this a bit more.

jacobq · 2018-03-12T22:28:09Z

I'll rebase this PR w/ your new commits on master in a moment. Feel free to edit anything however you like. All I really care about is that the test suite passes.

bendrucker

Left a lot of comments. I care most about getting the tests to a more readable place, then focusing on the source.

bendrucker · 2018-03-12T22:26:01Z

.travis.yml

 language: node_js
 node_js:
- iojs
+  - "4"


Please remove travis changes. I updated the node versions to 4/6/8. The npm test script is the default. And I don't want caching, that's for big projects.

bendrucker · 2018-03-12T22:27:36Z

.travis.yml

@@ -4,3 +4,11 @@ node_js:
  - 4
  - 6
  - 8
+
+cache:


Remove this, caching is for projects with huge dep graphs

bendrucker · 2018-03-12T22:28:10Z

.travis.yml

+    - $HOME/.npm
+    - node_modules
+
+script:


This is the default, remove it please. And generally don't change the CI config for a PR unless it's related (e.g. removing or adding node version support).

bendrucker · 2018-03-12T22:29:16Z

index.js

+//  - Any of the symbols ^*~<>|
+//  - A digit followed by .x
+//  - A hyphenated range like "a.b.c - d.e.f"
+const rangeRegEx = /^$|[\^*~<>|]|(\d+\.x)|(\d+(\.\d)*\s*-\s*\d+(\.\d)*)/


This regex is a lot hairier than the original and I'm not sure if I'm ok with munging all this logic into a single line. I'll come back to this.

You may want to consider eliminating the use of regex altogether. I think there are some standard functions in the semver package that might help.

Yeah that's what I meant

bendrucker · 2018-03-12T22:30:31Z

index.js

-module.exports = function exactVersion (version) {
-  return !regex.test(version)
+function extractSemVer (s) {
+  let result = null


Return early, don't declare "result" and conditionally assign to it

If you think that

function extractSemVer (s) { const match = s.match(nestedSemVerRegEx) return match && match[1] }

is more readable then so be it

bendrucker · 2018-03-12T22:34:12Z

test.js

+})
+
+test('range versions -> false', function (t) {
+  t.notOk(exact(''), 'Blank/empty is the same as *')


Empty string

Done. However, I think there is some merit to the idea of expressing why something is the way it is. There are quite a few "grey areas" regarding what an "exact" version is so there is potential for confusion. The test suite can serve to resolve any ambiguity.

bendrucker · 2018-03-12T22:35:18Z

test.js

+
+test('range versions -> false', function (t) {
+  t.notOk(exact(''), 'Blank/empty is the same as *')
+  t.notOk(exact('*'), '* means anything')


*

Same goes for all the other assertions. Don't write them like comments.

bendrucker · 2018-03-12T22:36:19Z

test.js

+test('range versions -> false', function (t) {
+  t.notOk(exact(''), 'Blank/empty is the same as *')
+  t.notOk(exact('*'), '* means anything')
+  t.notOk(exact('~4.0.0'), '~ specifies as range')


This test section is called range, keep the messages as concise as possible

bendrucker · 2018-03-12T22:40:14Z

test.js

+  '#semver:1.0.0'
+]
+
+test('exact commitish strings -> true', function (t) {


Move these to the main exact suite. There should be two test calls, one for exact, one for ranges.

Don't construct test assertions in a complex loop when you can avoid it. You can generate all your test "version" identifiers ahead of time using your URLs and refs.

bendrucker · 2018-03-12T22:41:03Z

test.js

+
+const exactRefs = [
+  '#master',
+  '#jj-abrams',


Pull this out, it's not distinct from #master

It contains a - which could be mistakenly identified as a range operator

If we wanted to make our lives harder we could test #1.0.0-1.2.0, which could be a valid (though misleading) exact tag but looks very similar to #semver:1.0.0-1.2.0 which would be a range.

bendrucker · 2018-03-13T00:40:15Z

test.js

+  '#semver:1.0.0'
+]
+
+const generateUrls = (baseURLs, refs) => {


Use a function declaration and put it at the end of the file. Don't use arrow functions except as one-liners or to bind this.

You can also do urls = in each test callback for the respective case. The idea is just that the test cases not be generated in a loop for easier debugging. If you call your helper twice that's still accomplished.

bendrucker · 2018-03-13T00:41:16Z

test.js

+
+const generateUrls = (baseURLs, refs) => {
+  return baseURLs.reduce((urls, baseURL) => {
+    return urls.concat(refs.map(ref => `${baseURL}${ref}`))


Avoid template strings when concatenation is clearer: baseURL + ref

bendrucker · 2018-03-13T00:42:03Z

index.js

+}
+
+module.exports = function exactVersion (versionString) {
+  if (typeof versionString !== 'string') {


Don't think we need to handle non-strings

bendrucker · 2018-03-13T00:49:42Z

index.js

+//  - Any of the symbols ^*~<>|
+//  - A digit followed by .x
+//  - A hyphenated range like "a.b.c - d.e.f"
+const rangeRegEx = /^$|[\^*~<>|]|(\d+\.x)|(\d+(\.\d)*\s*-\s*\d+(\.\d)*)/


Yeah that's what I meant

jacobq · 2018-11-02T21:54:36Z

Closing in favor of https://github.com/jacobq/is-exact-version

jacobq changed the title ~~Handle commitish strings & ".x"~~ Handle '', '*', '1.x.', 'URL#semver:...', etc. Mar 12, 2018

jacobq added 2 commits March 12, 2018 17:30

Handle commitish strings

1fbd492

Handle *, '', and hyphenated ranges

e270f67

jacobq force-pushed the feat/support-commitish branch from 50d11b4 to e270f67 Compare March 12, 2018 22:31

Refactor index.js with guard

8fd9579

bendrucker requested changes Mar 12, 2018

View reviewed changes

jacobq added 4 commits March 12, 2018 17:54

Refactor extractSemVer

97b6383

Refactor tests per bendrucker's request

4e15b5a

Refactor test suite exact/range URL generation

8575ed3

Refactor test suite into only two groups

da07ed9

bendrucker reviewed Mar 13, 2018

View reviewed changes

jacobq closed this Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle '', '*', '1.x.', 'URL#semver:...', etc. #1

Handle '', '*', '1.x.', 'URL#semver:...', etc. #1

jacobq commented Mar 12, 2018 •

edited

Loading

bendrucker commented Mar 12, 2018

jacobq commented Mar 12, 2018 •

edited

Loading

bendrucker commented Mar 12, 2018

jacobq commented Mar 12, 2018

bendrucker left a comment

bendrucker Mar 12, 2018

bendrucker Mar 12, 2018

bendrucker Mar 12, 2018

bendrucker Mar 12, 2018

jacobq Mar 12, 2018

bendrucker Mar 13, 2018

bendrucker Mar 12, 2018

jacobq Mar 12, 2018

bendrucker Mar 12, 2018

jacobq Mar 12, 2018

bendrucker Mar 12, 2018

bendrucker Mar 12, 2018

bendrucker Mar 12, 2018

bendrucker Mar 12, 2018

jacobq Mar 12, 2018

jacobq Mar 12, 2018

bendrucker Mar 13, 2018

bendrucker Mar 13, 2018

bendrucker Mar 13, 2018

bendrucker Mar 13, 2018

bendrucker Mar 13, 2018

jacobq commented Nov 2, 2018

@@ @@ -4,3 +4,11 @@ node_js: @@
                 - 4
                 - 6
                 - 8
+              cache:

Handle '', '*', '1.x.', 'URL#semver:...', etc. #1

Handle '', '*', '1.x.', 'URL#semver:...', etc. #1

Conversation

jacobq commented Mar 12, 2018 • edited Loading

bendrucker commented Mar 12, 2018

jacobq commented Mar 12, 2018 • edited Loading

bendrucker commented Mar 12, 2018

jacobq commented Mar 12, 2018

bendrucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobq commented Nov 2, 2018

jacobq commented Mar 12, 2018 •

edited

Loading

jacobq commented Mar 12, 2018 •

edited

Loading