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

Adding custom commit message option to gh_deploy #516

Merged
merged 7 commits into from May 15, 2015
Merged

Conversation

@mbacho
Copy link
Contributor

@mbacho mbacho commented May 11, 2015

Adds an option of configuring the commit message to be set when using gh_deploy

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 11, 2015

Code Health
Code quality remained the same when pulling f10d52d on mbacho:master into 4ad7529 on mkdocs:master.

@waylan
Copy link
Member

@waylan waylan commented May 11, 2015

Maybe there is something I'm missing here, but I don't follow this change. First, shouldn't the option passed to ghp-import be -m for the message, not -b (which is used for the branch)? Second, why is the message dependent on the config? Third, shouldn't the actual message be passed in on the command line, presumably by adding a new option to the cli.gh-deploy command?

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 11, 2015

Code Health
Code quality remained the same when pulling 47ab1c6 on mbacho:master into 4ad7529 on mkdocs:master.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 11, 2015

Yup, I agree with @waylan's comments. It should be added in the cli.py file and passed through.

Fix logging message to indicate correct remote branch to deploy to
@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 12, 2015

Code Health
Code quality remained the same when pulling 4138474 on mbacho:master into 4ad7529 on mkdocs:master.

@mbacho
Copy link
Contributor Author

@mbacho mbacho commented May 12, 2015

@waylan @d0ugal I agree with the first issue. It was my bad.
I also agree on the second and third issues. I updated the patch to get custom commit messages from cli.gh_deploy rather than the config.

@mbacho mbacho changed the title Adding 'ghp_message' option to gh_deploy Adding custom commit message option to gh_deploy May 12, 2015
@d0ugal d0ugal added this to the 0.13.0 milestone May 12, 2015
@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 12, 2015

Cool, looks good to me, will give it a second look over later and let anyone else that wants to review have a look.

command = ['ghp-import', '-p', config['site_dir']]

remote_branch = config.get('remote_branch', 'gh-pages')
command.extend(['-b', config['remote_branch']])

This comment has been minimized.

@waylan

waylan May 12, 2015
Member

Either this needs to be in an if statement (if config['remote_branch'] is not None:) or it needs to pass in the remote_branch var obtained in the previous line. That said, I don't see a remote_branch config setting defined in the defaults (which means it will never exist as unknown keys will raise an error). I suspect this may have been a bug in the existing code as well (at least after 9d3594c landed).

And I wonder if this is the right place to define the default. Perhaps the default should be left to the ghp-import tool. Thus the if not None suggestion -- a branch name should only be passed if one is defined.

I notice that you do use the remote_branch var later in the log message. Perhaps rather than assuming the default is "gh-pages", when a branch is not explicitly defined the message should simply say: "Copying <site_dir> to the default branch..." rather than actually naming the branch. In fact, if the remote_branch var is only used for the log message, seems like it should be calculated there. That way future reviewers of the code won't wonder why it is not used when building the command.

This comment has been minimized.

@d0ugal

d0ugal May 12, 2015
Member

Oh! The remote branch functionality will be currently broken as a side effect of #450. The old CLI parser had a one to one relationship between config settings and CLI arguments. This highlights the lack of test coverage in a few places like this.

So - we should add a remote branch argument like this PR adds message. @mbacho would you be happy to do that? Otherwise I can add it afterwards - I did break it after all 😄

This comment has been minimized.

@mbacho

mbacho May 12, 2015
Author Contributor

@waylan nice catch! I've updated the defaults in config. About the log, I would prefer the log informs the user what branch is being used, just for transparency. Thus the use of the branch name in the log.

@d0ugal since the remote_branch is less likely to change, unlike the commit message, I think config setting would suffice. However, I could make another PR to add it to CLI arguments, and maybe add tests for the gh_deploy module.

@@ -120,13 +122,14 @@ def json_command(clean, config_file, strict, site_dir):
@cli.command(name="gh-deploy")
@click.option('--clean', is_flag=True, help=clean_help)
@click.option('--config-file', type=click.File('rb'), help=config_file_help)
def gh_deploy_command(config_file, clean):
@click.option('--commit-message', is_flag=False, help=commit_message_help)

This comment has been minimized.

@waylan

waylan May 12, 2015
Member

I realize this is bikeshedding, but why not follow Git's pattern (like most Git add-ons do) and use --message. I would think all Git users will expect either that or -m. Of course, Click can support both, which is my personal preference (yes I realize mkdocs does not in most cases - to my annoyance). The point here is that --message would be the least surprising and the easiest to remember for users. The help text identifies it specifically as a commit message.

This comment has been minimized.

@d0ugal

d0ugal May 12, 2015
Member

Pull requests welcome to add short names :)

--message does sound like the best choice. Let's use that. You can add the short version of you like, otherwise it can be added with all the others later.

This comment has been minimized.

@mbacho

mbacho May 12, 2015
Author Contributor

I agree --message or -m is better than --commit-message. I've effected the change.

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 12, 2015

Code Health
Code quality remained the same when pulling 42c4e3d on mbacho:master into 4ad7529 on mkdocs:master.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 13, 2015

Okay, I mostly agree that remote branch can be a config option. However, we also need it to be a command line argument and if it is going to be a config option we also need to document it.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 14, 2015

Ignore the errors on Travis for now, they are unrelated to this change. I'm looking into it.

@mbacho
Copy link
Contributor Author

@mbacho mbacho commented May 14, 2015

@d0ugal I've added a CLI optional argument for remote branch and a fall back to the one set in config

@@ -120,13 +123,15 @@ def json_command(clean, config_file, strict, site_dir):
@cli.command(name="gh-deploy")
@click.option('--clean', is_flag=True, help=clean_help)
@click.option('--config-file', type=click.File('rb'), help=config_file_help)
def gh_deploy_command(config_file, clean):
@click.option('--message', '-m', help=commit_message_help)
@click.option('--branch', '-b', help=remote_branch_help)

This comment has been minimized.

@d0ugal

d0ugal May 14, 2015
Member

Call it --remote-branch to make it clear and that's the flag we have in the current version that got lost in a recent refactor.

"""Deply your documentation to GitHub Pages"""
config = load_config(
config_file=config_file
)
build.build(config, clean_site_dir=clean)
gh_deploy.gh_deploy(config)
gh_deploy.gh_deploy(config, message=message, branch=branch)

This comment has been minimized.

@d0ugal

d0ugal May 14, 2015
Member

Rather than passing the parameter here, it would be passed to load_config (see the other commands, this overwrites the config with CLI parameters and keeps them consistent). So just pass it as remote_branch as that is the config name.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 14, 2015

Two tiny final comments, and then I think this looks great - thanks very much!

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 14, 2015

Code Health
Code quality remained the same when pulling db167be on mbacho:master into 4ad7529 on mkdocs:master.

Rename CLI option 'branch' to 'remote-branch'
Pass CLI remote-branch option value to load_config instead of gh_deploy in cli.py
Update documentation
@mbacho
Copy link
Contributor Author

@mbacho mbacho commented May 15, 2015

@d0ugal changes made on the two issues.

@landscape-bot
Copy link

@landscape-bot landscape-bot commented May 15, 2015

Code Health
Code quality remained the same when pulling 0d2c63b on mbacho:master into 4ad7529 on mkdocs:master.

@d0ugal d0ugal added Enhancement and removed Update needed labels May 15, 2015
@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 15, 2015

Looks great, thanks! Just waiting for CI to pass and then I'll merge.

d0ugal added a commit that referenced this pull request May 15, 2015
Adding custom commit message option to gh_deploy
@d0ugal d0ugal merged commit 93a181a into mkdocs:master May 15, 2015
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 91.58%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.