-
-
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?
Conversation
JakobJingleheimer
left a comment
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.
Thanks for handling this :)
| 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 } | ||
| })) |
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?
| 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 } | |
| })) | |
| for (let i = 0; i < commits.length; i++) { | |
| if (!commit.id || !commit.message) { | |
| throw new Error('Each commit must have "id" and "message" properties') | |
| } | |
| args[i] = { sha: commit.id, ...commit } | |
| } |
|
|
||
| commitRun() | ||
| function run () { | ||
| if (parsed.tap) { |
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.
nit: I think the else is not necessary (just return) and only creates messier nesting.
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 output = compiledData.trim() | ||
| tt.match(output, | ||
| /# 69435db261/, | ||
| 'TAP output contains the sha of the commit being linted') |
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.
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.
| 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') |
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 likely to create bad DX. If it's not an array, just wrap it in an array.
The goal is to remove the need to clone the repo in https://github.com/nodejs/node/blob/main/.github/workflows/notify-on-push.yml, and instead rely on
push.commitslike so: