nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

feat: spawn the user's editor to edit commit messages #569

Closed targos closed 2 years ago

codecov[bot] commented 2 years ago

Codecov Report

Merging #569 (77a63d4) into main (25b452d) will decrease coverage by 2.25%. The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
- Coverage   82.45%   80.19%   -2.26%     
==========================================
  Files          35       37       +2     
  Lines        1755     1828      +73     
==========================================
+ Hits         1447     1466      +19     
- Misses        308      362      +54     
Impacted Files Coverage Δ
lib/utils.js 63.33% <20.00%> (-21.67%) :arrow_down:
lib/run.js 18.00% <0.00%> (ø)
lib/verbosity.js 61.53% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 25b452d...77a63d4. Read the comment docs.

targos commented 2 years ago

This could use some more manual testing.

I tested locally with:

EDITOR=vim -> Vim is opened, I can edit the message and when I save & close, the process continues with my message.

EDITOR=bad:

do you want to amend the commit message? Yes
--------------------------------- New Message ----------------------------------
meta: update AUTHORS

PR-URL: https://github.com/nodejs/node/pull/40392
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
--------------------------------------------------------------------------------
? Use this message? No
/bin/sh: bad: command not found
Failed to edit the message using the configured editor
Please manually edit /Users/targos/git/nodejs/node/.ncu/40392/6830c70b2edb-message, then run
`git commit --amend -F /Users/targos/git/nodejs/node/.ncu/40392/6830c70b2edb-message` to finish amending the message
targos commented 2 years ago

/cc @joyeecheung

Maybe we should do exactly like Git and also check for GIT_EDITOR and the core.editor config first?

joyeecheung commented 2 years ago

Taking the git configs into account SGTM

targos commented 2 years ago

I added support for GIT_EDITOR and git config core.editor.

I also tried with export EDITOR="code-insiders --wait" but that doesn't work and I have no idea why. The main process just exits with code 1 and no output in the console.

targos commented 2 years ago

Fixed!

Tested with

targos commented 2 years ago

/cc some people who land commits on a regular basis: @Trott @aduh95 @mhdawson @lpinca

targos commented 2 years ago

Ping. I'd be really happy to have this feature :)

joyeecheung commented 2 years ago

Speaking of which, would it make sense to add tests for these options?

I think we'd have to create some kind of infra for testing git node in general first - testing silent runs with --yes would be easier but testing interactive runs like this one requires would be difficult.

Speaking of which..have you tried running this with --yes and --autorebase? @targos

targos commented 2 years ago

Speaking of which..have you tried running this with --yes and --autorebase?

Yes, no problem with those options.