Skip to content

Conversation

@hewIngram
Copy link

Fixes: #466

Tweaks the verifyCurrentBranchIsMaster function to allow users to pass in a custom default branch name and restrict publishing to that branch only.

Question:
Do we want to default --any-branch on if users set --default-branch or is there another reason they might set one without the other?
I'm not familiar with Ava - is there a way to group tests like a describe block to group these verifyCurrentBranchIsDefault tests?

@itaisteinherz itaisteinherz changed the title feat: add flag to enforce publishing from only a specific, named branch Add --default-branch flag for specifying default branch Dec 20, 2019
@itaisteinherz
Copy link
Collaborator

@hewIngram Thanks for the PR, it looks great!
I left a couple of comments on things I wasn't sure about, but overall it's almost ready to be merged :)

@hewIngram
Copy link
Author

Thanks for the comments both and sorry for the delay.

Have made the above changes. I don't think he description in cli.js is perfect but I can't think of anything better for now.

anyBranch: {
type: 'boolean'
},
defaultBranch: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
defaultBranch: {
branch: {


exports.verifyCurrentBranchIsMaster = async () => {
if (await exports.currentBranch() !== 'master') {
exports.verifyCurrentBranchIsDefault = async (defaultBranch = 'master') => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to define a default value here, as it's set by meow (see this line).

if (await exports.currentBranch() !== 'master') {
exports.verifyCurrentBranchIsDefault = async (defaultBranch = 'master') => {
if (await exports.currentBranch() !== defaultBranch) {
throw new Error('Not on `master` branch. Use --any-branch to publish anyway.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
throw new Error('Not on `master` branch. Use --any-branch to publish anyway.');
throw new Error(`Not on \`{defaultBranch}\` branch. Use --any-branch to publish anyway, or define a different default branch using --branch.`);

await t.throwsAsync(gitUtil.verifyCurrentBranchIsDefault);
});

test('verifyCurrentBranchIsDefault doesn\'t throw if current branch the specified default branch', async t => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
test('verifyCurrentBranchIsDefault doesn\'t throw if current branch the specified default branch', async t => {
test('verifyCurrentBranchIsDefault doesn\'t throw if current branch is the specified default branch', async t => {

@itaisteinherz
Copy link
Collaborator

itaisteinherz commented Jan 14, 2020

Thanks @hewIngram! I left a few final comments, but overall this is almost ready to be merged 👌🏻

@itaisteinherz itaisteinherz changed the title Add --default-branch flag for specifying default branch Add --branch flag for specifying default branch Jan 14, 2020
@sindresorhus
Copy link
Owner

#476 (comment) still needs to be resolved.

@sindresorhus
Copy link
Owner

You also need to document it in the config section: https://github.com/sindresorhus/np#config

@sindresorhus
Copy link
Owner

@hewIngram Bump :)

@dadadidudu
Copy link

I created a pull request for the work done this pull request, it fixes the open documentation issues. I hope this will get merged into the project ASAP

@sindresorhus
Copy link
Owner

@dadadidudu Can you submit a PR here with the commits in this PR and also your additions? Doesn't look like hewIngram#1 will be merged.

@sindresorhus
Copy link
Owner

Closing this PR for lack of activity.

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.

Only publish from a specific branch

4 participants