-
-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add support for stdin JSON input #135
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,15 +65,21 @@ if (parsed.version) { | |
| if (!parsed.help && !args.length) { args.push('HEAD') } | ||
|
|
||
| function load (sha, cb) { | ||
| try { | ||
| const parsed = new URL(sha) | ||
| return loadPatch(parsed, cb) | ||
| } catch (_) { | ||
| exec(`git show --quiet --format=medium ${sha}`, (err, stdout, stderr) => { | ||
| if (err) return cb(err) | ||
| cb(null, stdout.trim()) | ||
| // Handle pre-parsed commit objects from stdin | ||
| if (typeof sha === 'object') { | ||
| return process.nextTick(() => { | ||
| cb(null, sha) | ||
| }) | ||
| } | ||
|
|
||
| const parsed = URL.parse(sha) | ||
| if (parsed != null) { | ||
| return loadPatch(parsed, cb) | ||
| } | ||
| exec(`git show --quiet --format=medium ${sha}`, (err, stdout, stderr) => { | ||
| if (err) return cb(err) | ||
| cb(null, stdout.trim()) | ||
| }) | ||
| } | ||
|
|
||
| function loadPatch (uri, cb) { | ||
|
|
@@ -121,33 +127,66 @@ if (parsed.list) { | |
| process.exit(0) | ||
| } | ||
|
|
||
| if (parsed.tap) { | ||
| const tap = new Tap() | ||
| tap.pipe(process.stdout) | ||
| if (parsed.out) tap.pipe(fs.createWriteStream(parsed.out)) | ||
| let count = 0 | ||
| const total = args.length | ||
|
|
||
| v.on('commit', (c) => { | ||
| count++ | ||
| const test = tap.test(c.commit.sha) | ||
| formatTap(test, c.commit, c.messages, v) | ||
| if (count === total) { | ||
| setImmediate(() => { | ||
| tap.end() | ||
| if (tap.status === 'fail') { process.exitCode = 1 } | ||
| }) | ||
| // Don't start processing if reading from stdin (handled in stdin.on('end')) | ||
| if (args.length === 1 && args[0] === '-') { | ||
| const chunks = [] | ||
| process.stdin.on('data', (chunk) => chunks.push(chunk)) | ||
| process.stdin.on('end', () => { | ||
| try { | ||
| const input = Buffer.concat(chunks).toString('utf8') | ||
| const commits = JSON.parse(input) | ||
|
|
||
| if (!Array.isArray(commits)) { | ||
| throw new Error('Input must be an array') | ||
| } | ||
|
|
||
| // Replace args with the commit data directly | ||
| args.splice(0, 1, ...commits.map(commit => { | ||
| if (!commit.id || !commit.message) { | ||
| throw new Error('Each commit must have "id" and "message" properties') | ||
| } | ||
| return { sha: commit.id, ...commit } | ||
| })) | ||
| run() | ||
| } catch (err) { | ||
| console.error('Error parsing JSON input:', err.message) | ||
| process.exit(1) | ||
| } | ||
| }) | ||
|
|
||
| tapRun() | ||
| process.stdin.resume() | ||
| } else { | ||
| v.on('commit', (c) => { | ||
| pretty(c.commit, c.messages, v) | ||
| commitRun() | ||
| }) | ||
| run() | ||
| } | ||
|
|
||
| commitRun() | ||
| function run () { | ||
| if (parsed.tap) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think the also nit: since the "is tap" block is larger, I would invert it to avoid nesting: if (!parsed.tap) {
v.on('commit', (c) => {
pretty(c.commit, c.messages, v)
commitRun()
})
return commitRun()
}
const tap = new Tap()
// … |
||
| const tap = new Tap() | ||
| tap.pipe(process.stdout) | ||
| if (parsed.out) tap.pipe(fs.createWriteStream(parsed.out)) | ||
| let count = 0 | ||
| const total = args.length | ||
|
|
||
| v.on('commit', (c) => { | ||
| count++ | ||
| const test = tap.test(c.commit.sha) | ||
| formatTap(test, c.commit, c.messages, v) | ||
| if (count === total) { | ||
| setImmediate(() => { | ||
| tap.end() | ||
| if (tap.status === 'fail') { process.exitCode = 1 } | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| tapRun() | ||
| } else { | ||
| v.on('commit', (c) => { | ||
| pretty(c.commit, c.messages, v) | ||
| commitRun() | ||
| }) | ||
|
|
||
| commitRun() | ||
| } | ||
| } | ||
|
|
||
| function tapRun () { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,5 +152,211 @@ test('Test cli flags', (t) => { | |
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with valid JSON', (tt) => { | ||
| const validCommit = { | ||
| id: '2b98d02b52', | ||
| message: 'stream: make null an invalid chunk to write in object mode\n\nthis harmonizes behavior between readable, writable, and transform\nstreams so that they all handle nulls in object mode the same way by\nconsidering them invalid chunks.\n\nPR-URL: https://github.com/nodejs/node/pull/6170\nReviewed-By: James M Snell <[email protected]>\nReviewed-By: Matteo Collina <[email protected]>' | ||
| } | ||
| const input = JSON.stringify([validCommit]) | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['-']) | ||
| let compiledData = '' | ||
| let errorData = '' | ||
|
|
||
| ls.stdout.on('data', (data) => { | ||
| compiledData += data | ||
| }) | ||
|
|
||
| ls.stderr.on('data', (data) => { | ||
| errorData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| tt.equal(code, 0, 'CLI exits with zero code on success') | ||
| tt.match(compiledData, /[^0-9a-f]2b98d02b52[^0-9a-f]/, 'output contains commit id') | ||
| tt.equal(errorData, '', 'no error output') | ||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with invalid commit (missing subsystem)', (tt) => { | ||
| const invalidCommit = { | ||
| id: 'def456', | ||
| message: 'this is a bad commit message without subsystem\n\nPR-URL: https://github.com/nodejs/node/pull/1234\nReviewed-By: Someone <[email protected]>' | ||
| } | ||
| const input = JSON.stringify([invalidCommit]) | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['-']) | ||
| let compiledData = '' | ||
|
|
||
| ls.stdout.on('data', (data) => { | ||
| compiledData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| tt.notEqual(code, 0, 'CLI exits with non-zero code on failure') | ||
| tt.match(compiledData, /def456/, 'output contains commit id') | ||
| tt.match(compiledData, /title-format/, 'output mentions the rule violation') | ||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with multiple commits', (tt) => { | ||
| const commits = [ | ||
| { | ||
| id: 'commit1', | ||
| message: 'doc: update README\n\nPR-URL: https://github.com/nodejs/node/pull/1111\nReviewed-By: Someone <[email protected]>' | ||
| }, | ||
| { | ||
| id: 'commit2', | ||
| message: 'test: add new test case\n\nPR-URL: https://github.com/nodejs/node/pull/2222\nReviewed-By: Someone <[email protected]>' | ||
| } | ||
| ] | ||
| const input = JSON.stringify(commits) | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['-']) | ||
| let compiledData = '' | ||
|
|
||
| ls.stdout.on('data', (data) => { | ||
| compiledData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| tt.equal(code, 0, 'CLI exits with zero code on success') | ||
| tt.match(compiledData, /commit1/, 'output contains first commit id') | ||
| tt.match(compiledData, /commit2/, 'output contains second commit id') | ||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with TAP output', (tt) => { | ||
| const validCommit = { | ||
| id: '69435db261', | ||
| message: 'chore: update tested node release lines (#94)' | ||
| } | ||
| const input = JSON.stringify([validCommit]) | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['--no-validate-metadata', '--tap', '-']) | ||
| let compiledData = '' | ||
|
|
||
| ls.stdout.on('data', (data) => { | ||
| compiledData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| const output = compiledData.trim() | ||
| tt.match(output, | ||
| /# 69435db261/, | ||
| 'TAP output contains the sha of the commit being linted') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: not sure if the current is required by the cumbersome lint rules of this repo, but the ) not being on its own line makes this very difficult to read. |
||
| tt.match(output, | ||
| /not ok \d+ subsystem: Invalid subsystem: "chore" \(chore: update tested node release lines \(#94\)\)/, | ||
| 'TAP output contains failure for subsystem') | ||
| tt.match(output, | ||
| /# fail\s+\d+/, | ||
| 'TAP output contains total failures') | ||
| tt.match(output, | ||
| /# Please review the commit message guidelines:\s# https:\/\/github.com\/nodejs\/node\/blob\/HEAD\/doc\/contributing\/pull-requests.md#commit-message-guidelines/, | ||
| 'TAP output contains pointer to commit message guidelines') | ||
| tt.equal(code, 1, 'CLI exits with non-zero code on failure') | ||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with invalid JSON', (tt) => { | ||
| const input = 'this is not valid JSON' | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['-']) | ||
| let errorData = '' | ||
|
|
||
| ls.stderr.on('data', (data) => { | ||
| errorData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| tt.equal(code, 1, 'CLI exits with non-zero code on error') | ||
| tt.match(errorData, /Error parsing JSON input/, 'error message is shown') | ||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with non-array JSON', (tt) => { | ||
| const input = JSON.stringify({ id: 'test', message: 'test' }) | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['-']) | ||
| let errorData = '' | ||
|
|
||
| ls.stderr.on('data', (data) => { | ||
| errorData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| tt.equal(code, 1, 'CLI exits with non-zero code on error') | ||
| tt.match(errorData, /Input must be an array/, 'error message is shown') | ||
|
Comment on lines
+310
to
+312
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems likely to create bad DX. If it's not an array, just wrap it in an array. |
||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with missing properties', (tt) => { | ||
| const input = JSON.stringify([{ id: 'test' }]) // missing 'message' | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['-']) | ||
| let errorData = '' | ||
|
|
||
| ls.stderr.on('data', (data) => { | ||
| errorData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| tt.equal(code, 1, 'CLI exits with non-zero code on error') | ||
| tt.match(errorData, /must have "id" and "message" properties/, 'error message is shown') | ||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.test('test stdin with --no-validate-metadata', (tt) => { | ||
| const commit = { | ||
| id: 'novalidate', | ||
| message: 'doc: update README\n\nThis commit has no PR-URL or reviewers' | ||
| } | ||
| const input = JSON.stringify([commit]) | ||
|
|
||
| const ls = spawn('./bin/cmd.js', ['--no-validate-metadata', '-']) | ||
| let compiledData = '' | ||
|
|
||
| ls.stdout.on('data', (data) => { | ||
| compiledData += data | ||
| }) | ||
|
|
||
| ls.stdin.write(input) | ||
| ls.stdin.end() | ||
|
|
||
| ls.on('close', (code) => { | ||
| tt.equal(code, 0, 'CLI exits with zero code when metadata validation is disabled') | ||
| tt.match(compiledData, /novalidate/, 'output contains commit id') | ||
| tt.end() | ||
| }) | ||
| }) | ||
|
|
||
| t.end() | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little convoluted. Could it be done with a simple loop?