Check that the change log has been updated as part of a PR #759
Conversation
rpowis
force-pushed
the
SDT-207-changelog-check
branch
from
ea9f956
to
b09f524
Compare
There was a problem hiding this comment.
Good stuff @rpowis
A few comments, mainly around self documenting variable names, a few more checks on tests and ways of writingPromise functions.
gulpfile.js/tasks/changelog.js
Outdated
| return Promise.reject(new Error('No CHANGELOG.md update')) | ||
| } | ||
|
|
||
| return Promise.resolve(true) |
There was a problem hiding this comment.
is there a need to return true here you can just Promise.resolve()?
There was a problem hiding this comment.
Shouldn't even be a Promise. See above comment
| } | ||
|
|
||
| var checkForChangelog = function (files) { | ||
| if (!files.includes('CHANGELOG.md')) { |
There was a problem hiding this comment.
Potentially a preference thing but I would ask why are you using Promise.reject and Promise.resolve rather than just returning a return new Promise(function (resolve, reject){ ...? It feels like there is a mix of both ways in this file. IMO it would be good to choose one or the other? Feels like a shorthand too far rather than defining this as a Promise at the function/interface level. Thoughts?
There was a problem hiding this comment.
Yeah, there is a mix of styles. I just got bored of writing the boilerplate of return new when hammering this out and then forgot to go back and standardise on one. Oops
I disagree with it being "a shorthand too far" though. If anything these returns shouldn't even be Promises (as returning a value from a then callback is fine) but that was blowing up on Travis for some reason so "Promisify all the things" was the quickest/easiest way around it 😕
I'll go back and investigate further. Happy to pair on it if you want?
There was a problem hiding this comment.
If we need to "Promisify all the things" it feels for readability there should be one style
gulpfile.js/tasks/changelog.js
Outdated
| } else { | ||
| var cmd = 'git rev-parse HEAD' | ||
| runCommand(cmd) | ||
| .then(function (commit) { |
There was a problem hiding this comment.
this can just be .then(resolve)
| getCurrentCommit(process.env.TRAVIS_COMMIT) | ||
| .then(getChangedFiles) | ||
| .then(checkForChangelog) | ||
| .then(function () { |
There was a problem hiding this comment.
this can just be .then(done)
There was a problem hiding this comment.
No it can't because it will pass the return value to done(), making gulp think there's an error when there isn't one.
There was a problem hiding this comment.
that feels a little like if checkForChangelog did this it wouldn't? why does it need to return true? (I suspect something I'm not aware of)
var checkForChangelog = function (files) {
return new Promise((function (resolve, reject) {
if (!files.includes('CHANGELOG.md')) {
reject(new Error('No CHANGELOG.md update'))
}
resolve()
})
There was a problem hiding this comment.
What's there is easier to read than that IMO.
It returns true because it has to return something. But really, we should be able to get rid of the Promise entirely as then() returns a promise anyway.
gulpfile.js/tests/changelog.test.js
Outdated
|
|
||
| changelog.runCommand() | ||
| .catch(function (data) { | ||
| t.ok(data instanceof Error, 'returns an Error if the command fails') |
There was a problem hiding this comment.
worth also checking data.message/err.message here?
gulpfile.js/tests/changelog.test.js
Outdated
| }) | ||
|
|
||
| changelog.runCommand('echo "test"') | ||
| .then(function (command) { |
There was a problem hiding this comment.
command variable could/should be stdout
gulpfile.js/tests/changelog.test.js
Outdated
| var changedFiles = changelog.getChangedFiles('test') | ||
|
|
||
| noBranch.catch(function (err) { | ||
| t.ok(err instanceof Error, 'throws an error when not given a branch') |
There was a problem hiding this comment.
could you also check err.message here?
gulpfile.js/tests/changelog.test.js
Outdated
|
|
||
| changelog.checkForChangelog('CHANGELOG') | ||
| .catch(function (err) { | ||
| t.ok(err instanceof Error, 'returns an Error when CHANGELOG.md cannot be found') |
There was a problem hiding this comment.
could you also check err.message here?
There was a problem hiding this comment.
This is also a dupe of the previous comment
| getCurrentCommit(process.env.TRAVIS_COMMIT) | ||
| .then(getChangedFiles) | ||
| .then(checkForChangelog) | ||
| .then(function () { |
There was a problem hiding this comment.
Is this the same comment as above somehow? 😕
There was a problem hiding this comment.
hmmm not sure what has happened there?
| @@ -134,6 +134,7 @@ | |||
| "devDependencies": { | |||
| "gulp-standard": "^8.0.3", | |||
| "gulp-tape": "0.0.9", | |||
| "sinon": "^1.17.7", | |||
There was a problem hiding this comment.
is this (and all the other dev dependencies) going to cause HMRC prototype kit problems? May be worth moving them to dependencies but as a separate PR?
There was a problem hiding this comment.
Doesn't matter, HMRC Prototype Kit is deprecated. They're not prod dependencies so they're going in devDependencies as they rightly should.
|
Also this branch needs to rebase master onto it |
rpowis
force-pushed
the
SDT-207-changelog-check
branch
from
ef03e3b
to
d7ab7f2
Compare
rpowis
dismissed
feedmypixel’s stale review
Made the fixes that needed to be made. The remaining comments have coments
|
LGTM 👍 |
Problem
Having added a
CHANGELOG.mdwe need a way to remind contributors to update it when they make a pull request.Solution
Add a gulp task that breaks the build if
CHANGELOG.mdhasn't been updated as part of the pull request.CHANGELOG.mdappears in that list and throw an error if it doesFailing: https://travis-ci.org/hmrc/assets-frontend/builds/207461966#L1140:
Passing: https://travis-ci.org/hmrc/assets-frontend/builds/207466855#L1141