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.

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

Add support for custom commit message #597

Merged
merged 18 commits into from Feb 12, 2021

Conversation

nnmrts
Copy link
Contributor

@nnmrts nnmrts commented Feb 2, 2021

Fixes #590

@sindresorhus
Copy link
Owner

This needs to be added for the config too, not just CLI flag.

The default is not clear.

It should not have an alias.

It should be documented in the readme.

It needs a test.

@nnmrts
Copy link
Contributor Author

nnmrts commented Feb 2, 2021

This needs to be added for the config too, not just CLI flag.

The default is not clear.

It should not have an alias.

It should be documented in the readme.

It needs a test.

Yeah this is why this is a draft. Sorry, maybe I misunderstood the draft PR feature?

@nnmrts
Copy link
Contributor Author

nnmrts commented Feb 2, 2021

This needs to be added for the config too, not just CLI flag.

I'm not quite sure what you meant with that. The message flag implementation should work regardless of cli or config usage, right?

The default is not clear.

I hope the commits 3b94dd9 and 544075a made that clearer. I don't really know how I could make it more clear without making the inline description too long, any suggestions?

It should not have an alias.

Alright, f975e1e.

It should be documented in the readme.

fb56c72 adds that. Maybe there should be seperate section for the message flag in the readme though, because yarn and npm handle it wildly different and have different defaults too. And yarn 2 doesn't even support it, as far as I know. Any thoughts

It needs a test.

I'm not quite sure, but since everything else is tested by either meow, npm or yarn, I think the only test we would need here is a check if the message flag adds -m 'message' to the 'npm version' command. I don't really know how to check for commands sent out by np in the test. I've seen mockery and execa and sinon stubs, but I don't really understand these things and how they are integrated here. Can you help me with that?

I'll make this PR ready for review now, I hope that's okay.

@nnmrts nnmrts marked this pull request as ready for review February 2, 2021 06:54
@nnmrts
Copy link
Contributor Author

nnmrts commented Feb 2, 2021

Some extra thoughts: A prompt for the commit message would be nice too, in a future pull request. We could either check if the message flag is added but empty, and if it is, prompt the user to input a commit message. Or we introduce a new flag if the user wants to get prompted for the commit message instead of using the current message flag. Something like --prompt-for-message or --commit-prompt, idk.

source/index.js Outdated Show resolved Hide resolved
source/index.js Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Yeah this is why this is a draft. Sorry, maybe I misunderstood the draft PR feature?

I didn't mean to imply you didn't plan to do this. I was just trying to save you time by making it clear what needs to be done.

@sindresorhus
Copy link
Owner

Some extra thoughts: A prompt for the commit message would be nice too, in a future pull request.

Why? I'm fairly against that unless there's a very good use-case. I'm trying really hard to not end up with hundreds of flags like npm and yarn has. It's important to be a little bit opinionated.

@nnmrts
Copy link
Contributor Author

nnmrts commented Feb 6, 2021

I didn't mean to imply you didn't plan to do this. I was just trying to save you time by making it clear what needs to be done.

Alright, thanks for clearing that up, I was just a bit tired at that time and miserunderstood it.

Why? I'm fairly against that unless there's a very good use-case. I'm trying really hard to not end up with hundreds of flags like npm and yarn has. It's important to be a little bit opinionated.

That's okay, I appreciate that, was just brainstorming. What do you think about the discussion here: #597 (comment)?

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
Co-authored-by: Govind S <gvind4@gmail.com>
@sindresorhus sindresorhus changed the title Add custom commit message support Add support for custom commit message Feb 12, 2021
@sindresorhus sindresorhus merged commit 65a674e into sindresorhus:main Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Custom commit message input
3 participants