Allow specifying the base branch #30
Conversation
greglockwood
changed the title
| # Intellij IDE project settings | ||
| .idea | ||
| # Because dogfooding | ||
| .pr-train.yml |
There was a problem hiding this comment.
Feel free to remove these changes, or undo them. I just needed to make them to have a clean working directory when rebasing and so on.
cfg_template.yml
Outdated
| @@ -1,3 +1,7 @@ | |||
| prs: | |||
| # Change this if you use a different name for your default branch (e.g. main) | |||
| main-branch-name: master | |||
There was a problem hiding this comment.
Should this be commented out by default and have a different value to the hardcoded default, by way of example?
| function checkAndReportInvalidBaseError(e, base) { | ||
| const { field, code } = get(e, 'body.errors[0]', {}); | ||
| if (field === 'base' && code === 'invalid') { | ||
| console.log([ | ||
| emoji.get('no_entry'), | ||
| `\n${emoji.get('confounded')} This is embarrassing. `, | ||
| `The base branch of ${base.bold} doesn't seem to exist on the remote.`, | ||
| `\nDid you forget to ${emoji.get('arrow_up')} push it?`, | ||
| ].join('')); | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This caters for scenarios where you are trying to specify a base branch that hasn't been pushed to the remote yet. The intention is to try to help out scatter-brained developers if this scenario does arise.
It certainly did for me, so I added this helpful suggestion to shortcut fixing the problem if anyone else encounters it in the future.
I'm happy to reword it if you feel the tone is inconsistent.
| @@ -163,11 +186,14 @@ async function ensurePrsExist(sg, allBranches, combinedBranch, remote = DEFAULT_ | |||
| title, | |||
| body, | |||
| }; | |||
| process.stdout.write(`Creating PR for branch "${branch}"...`); | |||
| const baseMessage = base === baseBranch ? colors.dim(` (against ${base})`) : ''; | |||
There was a problem hiding this comment.
Having the base branch in the console output is helpful for visually confirming what base branch is being used since it can now change.
| const sg = simpleGit(); | ||
| if (!(await sg.checkIsRepo())) { | ||
| console.log('Not a git repo'.red); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // try to create or init the config first so we can read values from it | ||
| if (process.argv.includes('--init')) { | ||
| if (fs.existsSync(await getConfigPath(sg))) { | ||
| console.log('.pr-train.yml already exists'); | ||
| process.exit(1); | ||
| } | ||
| const root = path.dirname(require.main.filename); | ||
| const cfgTpl = fs.readFileSync(`${root}/cfg_template.yml`); | ||
| fs.writeFileSync(await getConfigPath(sg), cfgTpl); | ||
| console.log(`Created a ".pr-train.yml" file. Please make sure it's gitignored.`); | ||
| process.exit(0); | ||
| } | ||
|
|
||
| let ymlConfig; | ||
| try { | ||
| ymlConfig = await loadConfig(sg); | ||
| } catch (e) { | ||
| if (e instanceof yaml.YAMLException) { | ||
| console.log('There seems to be an error in `.pr-train.yml`.'); | ||
| console.log(e.message); | ||
| process.exit(1); | ||
| } | ||
| console.log('`.pr-train.yml` file not found. Please run `git pr-train --init` to create one.'.red); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
By moving this block here, I was able to use the values read from the config to inform the command line parameters added or their default values.
| @@ -309,6 +337,6 @@ async function main() { | |||
| } | |||
|
|
|||
| main().catch(e => { | |||
| console.log(`${emoji.get('x')} An error occured. Was there a conflict perhaps?`.red); | |||
| console.log(`${emoji.get('x')} An error occurred. Was there a conflict perhaps?`.red); | |||
There was a problem hiding this comment.
I threw in a bonus spelling mistake fix.
There was a problem hiding this comment.
This is truly awesome, thanks @greglockwood! Just two super minor suggestions and we're ready to merge.
|
I'll have a look at the other PRs in the next days! Looking at my code from two years ago, I really wish I:
:) This tool was a bit of a proof of concept to see if I could make things easier (for myself and possibly for others)...so I'm happy to say it mostly worked out OK. :) Thanks again for the PRs, I really appreciate it. |
Which problem does this PR solve?
There are a few scenarios not covered by the current functionality.
master, meaning the base should be set to the "parent" branch of the first branch in the chain, notmaster.master. New repos default to usingmaininstead.Main changes
1. Add
--baseto allow customizing the base branchThe first part was adding a new command line flag and propagating it through the functions to allow specifying a custom base branch to use other than
master.2. Adding a configuration option to specify a default base branch
This is the first PR in a chain because I started a new convention where configuration options about the behavior can be specified in an optional
prssection in the.pr-train.ymlconfig file. By chaining the remaining PRs off this one, I can re-use the helper method for getting the config value.Most of the PRs in the chain add a configuration option to this section, and this one is no different. This one adds a
main-base-branchsetting to allow customizing the main branch for the repository you are working on, which fixes issue #24 and may help with #18 and #10.Specifying this
main-base-branchoption sets the base branch to use when one is not specified explicitly with the--basecommand-line option mentioned above.Suggested CR ordering of files?
consts.jsandcfg_template.yml, the "easy" ones.README.mdfor the addition to the docs.index.jsgithub.jsHow I tested it works
I have tested the following scenarios:
main-base-branchsetting, no--basespecified via command-line. First and combined PRs usemasteras the base, which matches the current behavior.main-base-branch: main, no--basespecified via command-line. First and combined PRs usemainas the base, which is correct.main-base-branch: mainand--base masterspecified via command-line. First and combined PRs usemasteras the base, which is correct.main-base-branchsetting,--base mainspecified via command-line. First and combined PRs usemainas the base, which is correct.