switch from Node v5.x.x to Node v6.x.x #250
Conversation
| @@ -60,6 +60,6 @@ setup: | |||
| test: | |||
| $(ISTANBUL) cover node_modules/.bin/_mocha -- --recursive --timeout 10000 | |||
| $(ISTANBUL) check-coverage --branches 100 | |||
| ifneq ($(shell node --version | sed 's/[.][^.]*$$//'),v0.10) | |||
| ifeq ($(shell node --version | cut -d . -f 1),v6) | |||
There was a problem hiding this comment.
Perhaps this belongs in the issue, but (assuming makefiles have a concept of else), it might be appropriate to place a message here? That way, a contributor will realise if doctests didn't run and they may need to fix something before opening a PR
There was a problem hiding this comment.
Very good idea! I'll make a change so that a warning message is emitted in the else branch. :)
There was a problem hiding this comment.
Updated. See below. Sample output:
⚠️ Doctests are only run in Node v6.x.x (current version is v4.5.0)
There was a problem hiding this comment.
Great! I often run in a terminal without escape code support, but the string of escape codes would at least draw my attention :)
There was a problem hiding this comment.
Good point. It doesn't print properly in the CircleCI logs either. I'll use colour codes instead:
$ echo $'\e[33m\e[7mWARN\e[27m Doctests are only run in Node v6.x.x (current version is '"$(node --version)"$')\e[0m'There was a problem hiding this comment.
My primary environment (currently) doesn't actually support any escape codes, but I still think they're a good idea here. (If you were very worried about this, you could have a $TERM=dumb check)
There was a problem hiding this comment.
After some trial and error, I think I have a version which will do the right thing in most environments:
- If
$(TERM)isdumb,$(patsubst dumb,,$(TERM))will evaluate to empty string; otherwise it will evaluate to the value of$(TERM)(which may be empty string). $(if $(patsubst dumb,,$(TERM)),\033[33m\033[7mWARN\033[27m,[WARN])will evaluate to[WARN]if the value of$(TERM)isdumb(or empty string);\033[33m\033[7mWARN\033[27motherwise.33msets the foreground colour to yellow,7mswitches foreground and background colours, and27mundoes the switch.- This appears inside a
$''string, but since$has special meaning in a makefile we must escape it with an extra dollar sign.
@rjmk, would you mind sanity checking the line below? There's no doubt we're overengineering this, but I believe the only cost is the time I've spent fiddling (which I've quite enjoyed).
One can test this by commenting out lines 61 and 62 then running make test and TERM=dumb make test. CircleCI is happy now too.
There was a problem hiding this comment.
Actually, I decided to restrict myself to ASCII regardless of environment. I had too much trouble getting CircleCI to do the right thing. Also, I'm now redirecting the output to stderr.
|
👍 |
davidchambers
force-pushed
the
dc-node-version
branch
22 times, most recently
from
000e1d9 to
406c255
Compare
davidchambers
force-pushed
the
dc-node-version
branch
from
406c255 to
8046e60
Compare
This addresses the test failure noted in #248.
As can be seen in the diff, we currently have a guard which prevents doctests from being run in Node v0.10.x (which doesn't support arrow functions). It's completely reasonable to run doctests in just one environment, since their purpose is to keep the documentation up to date rather than guarantee compatibility across Node versions. I suggest the latest stable version if we're to choose just one.
I hope these changes obviate the need to mention a version in CONTRIBUTING.md, as it would be easy for such a note to become out of date. Let's merge this pull request then discuss in #248 whether to add a note.