search

SamVerschueren / clinton

Project style linter
MIT License
122 stars 9 forks source link

Skip nvm aliases in Travis CI rule #64

Closed sonicdoe closed 7 years ago

sonicdoe commented 7 years ago

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 commented 7 years ago

Coverage Status

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

SamVerschueren commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

SamVerschueren commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Coverage Status

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

SamVerschueren commented 7 years ago

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

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

coveralls commented 7 years ago

Coverage Status

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

sonicdoe commented 7 years ago

Done.

coveralls commented 7 years ago

Coverage Status

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

SamVerschueren commented 7 years ago

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