Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: skip fixup! and squash! commits #33

Closed
wants to merge 5 commits into from
Closed

lib: skip fixup! and squash! commits #33

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79f946e
lib: skip fixup! and squash! commits
richardlau Nov 9, 2018
d07d944
squash! lib: skip fixup! and squash! commits
richardlau Nov 10, 2018
25e5398
fixup! lib: skip fixup! and squash! commits
richardlau Nov 10, 2018
47f98de
fixup! lib: skip fixup! and squash! commits
richardlau Nov 10, 2018
229ba0c
fixup! lib: skip fixup! and squash! commits
richardlau Nov 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ module.exports = class ValidateCommit extends EE {
this.disableRule('reviewers')
this.disableRule('metadata-end')
}
this.disableRule('skip-fixup-squash')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be disabled with a CLI option (see this.opts above) by default? I think only our CI will actually need this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it skip-fixup-squash should be disabled by default. If node-core-utils detects a fixup or squash commit, I'd want it flagged. It's just CI where we want it disabled. I consider node-core-utils the primary use case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Or check-fixup-squash is enabled by default and the --no-check-fixup-squash flag disables it.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll rework to --check-autosquash (less to type than --check-fixup-squash).

}

disableRule(id) {
Expand All @@ -47,9 +48,14 @@ module.exports = class ValidateCommit extends EE {
}
} else {
const commit = new Parser(str, this)
for (const rule of this.rules.values()) {
if (rule.disabled) continue
rule.validate(commit)
if (commit.title.startsWith('fixup!') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file seems unnecessary? I think the request was more like, if an option is on, then enable this rule, then check the fixup! and squash!. The rule disabling logic is in lib/index.js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way it works is the options are positive (e.g., validate-metadata) and prepended with no at the command line to disable (e.g., --no-validate-metadata). So this might be better as a check-fixup-squash or check-autosquashable option and then our CI can send --no-lint-fixup-squash or --no-lint-autosquashable?

Copy link
Member Author

@richardlau richardlau Nov 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file are because, AFAICT, the rules as currently implemented are independent of each other and unordered. The new "rule" is there to give visual feedback when the commit is skipped, e.g.

-bash-4.2$ node bin/cmd.js --no-validate-metadata https://api.github.com/repos/nodejs/node/pulls/24254/commits
  ✔  1a79e37ebc0407581c093fac70a541d4d7b77ad5
     ✔  0:0      skipping fixes-url                        fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✔  0:0      valid subsystems                          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ✔  0:0      Title is <= 50 columns.                   title-length
  ✔  295c5cfef7a0dc36069d6e2ae523c2ed11ce3d1b
     ✔  0:0      Skipping fixup! commit.                   skip-fixup-squash
  ✔  5a01d5e5664889e8a8d1febf6b01b70379948c38
     ✔  0:0      Skipping fixup! commit.                   skip-fixup-squash
  ✔  2857dbb485c31df99792171ed5a788121dc011a6
     ✔  0:0      Skipping fixup! commit.                   skip-fixup-squash
-bash-4.2$

commit.title.startsWith('squash!')) {
this.rules.get('skip-fixup-squash').validate(commit)
} else {
for (const rule of this.rules.values()) {
if (rule.disabled) continue
rule.validate(commit)
}
}

setImmediate(() => {
Expand Down
43 changes: 43 additions & 0 deletions lib/rules/skip-fixup-squash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict'

const id = 'skip-fixup-squash'

module.exports = {
id: id
, meta: {
description: 'skip fixup! and squash! commits'
, recommended: true
}
, defaults: { }
, options: { }
, validate: (context, rule) => {
if (context.title.startsWith('fixup!')) {
context.report({
id: id
, message: 'Skipping fixup! commit.'
, string: context.title
, level: 'pass'
})
return
}

if (context.title.startsWith('squash!')) {
context.report({
id: id
, message: 'Skipping squash! commit.'
, string: context.title
, level: 'pass'
})
return
}

context.report({
id: id
, message: 'Commit is neither fixup! nor squash!.'
, string: context.title
, line: 0
, column: 0
, level: 'fail'
})
}
}
70 changes: 70 additions & 0 deletions test/validator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,27 @@ Date: Thu Mar 3 10:10:46 2016 -0600
test: Check memoryUsage properties.
`

const str10 = `commit 9af419a04237c3d158e71ecde9e04bbc2e77de1f
Author: Richard Lau <riclau@uk.ibm.com>
Date: Fri Nov 9 11:24:33 2018 -0500

skip fixup! and squash! commits
`

const str11 = `commit 1fd0e4375ea3ecb62d847827365d95e63bfe03f2
Author: Richard Lau <riclau@uk.ibm.com>
Date: Fri Nov 9 11:26:11 2018 -0500

fixup! skip fixup! and squash! commits
`

const str12 = `commit 1fd0e4375ea3ecb62d847827365d95e63bfe03f2
Author: Richard Lau <riclau@uk.ibm.com>
Date: Fri Nov 9 11:26:11 2018 -0500

squash! skip fixup! and squash! commits
`

test('Validator - misc', (t) => {
const v = new Validator()

Expand Down Expand Up @@ -344,5 +365,54 @@ test('Validator - real commits', (t) => {
})
})

t.test('title contains but does not start with fixup! and squash!', (tt) => {
const v = new Validator({
'validate-metadata': false
})
v.lint(str10)
v.on('commit', (data) => {
const msgs = data.messages
const filtered = msgs.filter((item) => {
return item.level === 'fail'
})
tt.equal(filtered.length, 1, 'messages.length')
tt.equal(filtered[0].message, 'Missing subsystem.')
tt.end()
})
})

t.test('fixup! commits are skipped', (tt) => {
const v = new Validator({
'validate-metadata': false
})
v.lint(str11)
v.on('commit', (data) => {
const msgs = data.messages
const filtered = msgs.filter((item) => {
return item.level === 'fail'
})
tt.equal(filtered.length, 0, 'reported failures')
tt.equal(msgs.length, 1, 'messages.length')
tt.equal(msgs[0].message, 'Skipping fixup! commit.')
tt.end()
})
})

t.test('squash! commits are skipped', (tt) => {
const v = new Validator({
'validate-metadata': false
})
v.lint(str12)
v.on('commit', (data) => {
const msgs = data.messages
const filtered = msgs.filter((item) => {
return item.level === 'fail'
})
tt.equal(filtered.length, 0, 'reported failures')
tt.equal(msgs.length, 1, 'messages.length')
tt.equal(msgs[0].message, 'Skipping squash! commit.')
tt.end()
})
})
t.end()
})