Skip nvm aliases in Travis CI rule #64

sonicdoe · 2017-01-26T18:50:10Z

When running clinton against a package with an engines field and node in .travis.yml it crashes with TypeError: Invalid Version: node.0.0. A similar error was already reported in #46.

With this change it now skips all nvm aliases in isSupported(). It still crashes when using the unstable alias but I suggest resolving this by adding unstable to the list of deprecated versions.

coveralls · 2017-01-26T18:54:05Z

Coverage increased (+0.02%) to 96.302% when pulling b0179d0 on sonicdoe:skip-nvm-aliases into dc90ac0 on SamVerschueren:master.

SamVerschueren · 2017-01-26T20:57:38Z

And to which version is node mapped? Latest? As you can see in this line, stable for instance is marked as deprecated. So actually I'm more in favour of marking these versions as not supported because it's not explicit about which versions it tests. Not really sure about it though.

@sindresorhus any remarks?

sonicdoe · 2017-01-26T21:12:12Z

stable was deprecated because it “only truly applies to node v0.12 and earlier”. node always maps to the latest version of Node.js.

I am a bit more in favor of ignoring the node alias as opposed to marking it as an error because it is useful to always test against the latest Node.js version. However, I can understand if users should be urged to keep their .travis.yml up to date.

sindresorhus · 2017-01-28T05:22:58Z

I agree node should just be ignored. It can be useful sometimes when you just want the latest Node.js version. Example.

SamVerschueren · 2017-01-30T08:04:43Z

Good point. @sonicdoe do you happen to have a reference of unstable being part of Travis? Can't seem to find it.

Let's ignore node. which seems it was already ignored but somehow fails on some other scenarios which aren't tested. All the other should be moved to the deprecated versions array, which only is the unstable version. But I want proof first of that alias to exist.

In order to ignore node decently, I'd suggest just using filter on the versions array so we don't have to take that one into account further down in the code.

sonicdoe · 2017-01-30T21:05:44Z

Travis CI uses nvm:

These values are passed on to nvm; newer releases not shown above may be used if nvm recognizes them.

nvm itself lists node, iojs, stable, and unstable as special default aliases. I have also just realized that there are LTS aliases. Given that these are similar to node, I have added them to the ignore array, too.

I have updated this pull request and opened another one deprecating unstable.

coveralls · 2017-01-30T21:06:59Z

Coverage increased (+0.02%) to 96.307% when pulling 113249e on sonicdoe:skip-nvm-aliases into dc90ac0 on SamVerschueren:master.

sonicdoe · 2017-01-30T21:11:01Z

rules/travis.js

+				}
+			}
+
+			versions = versions.filter(v => DEPRECATED_VERSIONS.indexOf(v) === -1);


I could have merged this filter with the above for…of loop, however, I think that the code is more readable this way and the performance impact is negligible.

This is good!

I'm looking back at the code and wondering why we can't write the above for loop like what we had before.

for (const version of versions) { if (DEPRECATED_VERSIONS.indexOf(version) !== -1) { ctx.report({ message: `Version \`${version}\` is deprecated.`, file }); } else if (engine && !semver.satisfies(normalize(version), engine)) { ctx.report({ message: `Unsupported version \`${version}\` is being tested.`, fix: fixers.unsupported(version), file }); } }

I believe this does the same thing as your code with 2 for-loops without having to filter out the deprecated versions.

We would then have to filter out the deprecated versions again in the if (engine) block further down. Like you said:

In order to ignore node decently, I'd suggest just using filter on the versions array so we don't have to take that one into account further down in the code.

Oh yes, you're right. Thanks for clarifying.

Although, maybe it might be a better idea to merge the two for..of loops and filter the versions array again after that for..of loop right before we enter the if (engine) block. I think it's just weird that we would do it in two loops while we can easily do it in one loop.

What do you think about merging them all into one filter?

let versions = Array.isArray(travis.node_js) ? travis.node_js : [travis.node_js]; versions = versions.filter(version => { if (IGNORED_VERSIONS.indexOf(version) !== -1) { return false; } else if (DEPRECATED_VERSIONS.indexOf(version) !== -1) { ctx.report({ message: `Version \`${version}\` is deprecated.`, file }); return false; } else if (engine && !semver.satisfies(normalize(version), engine)) { ctx.report({ message: `Unsupported version \`${version}\` is being tested.`, fix: fixers.unsupported(version), file }); } return true; });

This is what I initially tried to avoid but, on second thought, it might not be so bad.

Looks good to me!

SamVerschueren · 2017-01-31T13:48:52Z

I have updated this pull request and opened another one deprecating unstable.

Just deprecate unstable in this PR instead of a new one.

SamVerschueren

Could you also add extra tests for the lts aliases. Also, if you moved #65 into this one, add an extra test for unstable.

SamVerschueren · 2017-01-31T13:50:14Z

rules/travis.js

@@ -74,15 +75,23 @@ module.exports = ctx => {
 				});
 			}

-			const versions = Array.isArray(travis.node_js) ? travis.node_js : [travis.node_js];
+			let versions = Array.isArray(travis.node_js) ? travis.node_js : [travis.node_js];
+


Remove the empty line

coveralls · 2017-02-02T11:49:39Z

Coverage increased (+0.02%) to 96.307% when pulling b088fe0 on sonicdoe:skip-nvm-aliases into dc90ac0 on SamVerschueren:master.

sonicdoe · 2017-02-02T11:50:20Z

Done.

SamVerschueren

I'm also wondering if we should assign a semver version to the LTS aliases like this

const ALIASES = {
    'lts/argon': '4',
    'lts/boron': '6'
};

If someone sets the node engines to >=6 and he is testing for lts/argon we can exit and show an error that lts/argon is not supported.

But this might be out of reach of this PR. We could create a new issue for that afterwards.

SamVerschueren · 2017-02-02T11:55:56Z

rules/travis.js

+				}
+			}
+
+			versions = versions.filter(v => DEPRECATED_VERSIONS.indexOf(v) === -1);


I'm looking back at the code and wondering why we can't write the above for loop like what we had before.

for (const version of versions) { if (DEPRECATED_VERSIONS.indexOf(version) !== -1) { ctx.report({ message: `Version \`${version}\` is deprecated.`, file }); } else if (engine && !semver.satisfies(normalize(version), engine)) { ctx.report({ message: `Unsupported version \`${version}\` is being tested.`, fix: fixers.unsupported(version), file }); } }

I believe this does the same thing as your code with 2 for-loops without having to filter out the deprecated versions.

coveralls · 2017-02-02T17:28:41Z

Coverage increased (+0.03%) to 96.312% when pulling 84fb5c3 on sonicdoe:skip-nvm-aliases into dc90ac0 on SamVerschueren:master.

SamVerschueren · 2017-02-02T17:58:02Z

Awesome, great work @sonicdoe! I created a follow-up issue #66 for an extra improvement on this rule.

sonicdoe mentioned this pull request Jan 30, 2017

Deprecate unstable on Travis CI #65

Closed

sonicdoe commented Jan 30, 2017

View reviewed changes

SamVerschueren requested changes Jan 31, 2017

View reviewed changes

Deprecate unstable

451de2d

SamVerschueren requested changes Feb 2, 2017

View reviewed changes

Ignore nvm aliases

84fb5c3

SamVerschueren merged commit 40518ca into SamVerschueren:master Feb 2, 2017

sonicdoe deleted the skip-nvm-aliases branch February 2, 2017 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip nvm aliases in Travis CI rule #64

Skip nvm aliases in Travis CI rule #64

sonicdoe commented Jan 26, 2017

coveralls commented Jan 26, 2017 •

edited

Loading

SamVerschueren commented Jan 26, 2017 •

edited

Loading

sonicdoe commented Jan 26, 2017 •

edited

Loading

sindresorhus commented Jan 28, 2017

SamVerschueren commented Jan 30, 2017

sonicdoe commented Jan 30, 2017

coveralls commented Jan 30, 2017 •

edited

Loading

sonicdoe Jan 30, 2017

SamVerschueren Jan 31, 2017

SamVerschueren Feb 2, 2017

sonicdoe Feb 2, 2017

SamVerschueren Feb 2, 2017

sonicdoe Feb 2, 2017

SamVerschueren Feb 2, 2017

SamVerschueren commented Jan 31, 2017

SamVerschueren left a comment

SamVerschueren Jan 31, 2017

coveralls commented Feb 2, 2017 •

edited

Loading

sonicdoe commented Feb 2, 2017

SamVerschueren left a comment

SamVerschueren Feb 2, 2017

coveralls commented Feb 2, 2017 •

edited

Loading

SamVerschueren commented Feb 2, 2017

Skip nvm aliases in Travis CI rule #64

Skip nvm aliases in Travis CI rule #64

Conversation

sonicdoe commented Jan 26, 2017

coveralls commented Jan 26, 2017 • edited Loading

SamVerschueren commented Jan 26, 2017 • edited Loading

sonicdoe commented Jan 26, 2017 • edited Loading

sindresorhus commented Jan 28, 2017

SamVerschueren commented Jan 30, 2017

sonicdoe commented Jan 30, 2017

coveralls commented Jan 30, 2017 • edited Loading

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

SamVerschueren commented Jan 31, 2017

SamVerschueren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 2, 2017 • edited Loading

sonicdoe commented Feb 2, 2017

SamVerschueren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 2, 2017 • edited Loading

SamVerschueren commented Feb 2, 2017

coveralls commented Jan 26, 2017 •

edited

Loading

SamVerschueren commented Jan 26, 2017 •

edited

Loading

sonicdoe commented Jan 26, 2017 •

edited

Loading

coveralls commented Jan 30, 2017 •

edited

Loading

coveralls commented Feb 2, 2017 •

edited

Loading

coveralls commented Feb 2, 2017 •

edited

Loading