VCS configuration via command line parameters

[timobrembeck]

This PR adds the feature suggested in #168:

• Add the following flags to the bumpver update command:
  • --commit / --no-commit
  • --tag-commit / --no-tag-commit
  • --push / --no-push
• Add test cases for these flags including checks for incorrect usage
• Document new flags in README

I waived short options because -c, -t and -p are already in use and I couldn't think of other letters which don't lead to confusion.

When setting up my dev environment, I got a few pylint errors which are due to a new warning class introduced in Pylint 2.11. I fixed them by converting a few .format() calls to f-strings. I was a little bit confused because the library claims to support Python 2.7, but f-strings don't work there, do they? But I found several other occurrences of f-strings, so I thought it might be fine to convert the old syntax.

@mbarkhau thanks in advance for your feedback!

[mbarkhau]

Nice work, very well done :100:

so I thought it might be fine to convert the old syntax.

Correct, the project uses lib3to6 for support of py27 and your tests seem to have passed there

[mbarkhau]

I think it makes sense to do a cascade, instead of requiring all three flags. In fact that is already the default behaviour in the vcs.commit function.

# src/bumpver/vcs.py
def commit(
    cfg           : config.Config,
    vcs_api       : VCSAPI,
    filepaths     : typ.Set[str],
    new_version   : str,
    commit_message: str,
) -> None:
    if cfg.commit
        for filepath in filepaths:
            vcs_api.add(filepath)

        vcs_api.commit(commit_message)

    if cfg.commit and cfg.tag:
        vcs_api.tag(new_version)

    if cfg.commit and cfg.tag and cfg.push:
        vcs_api.push(new_version)

Can you say something about your thought process for not just doing this?

# src/bumpver/cli.py
def _parse_vcs_options(
    cfg   : config.Config,
    commit: typ.Optional[bool] = None,
    tag   : typ.Optional[bool] = None,
    push  : typ.Optional[bool] = None,
) -> config.Config:
    if commit is not None:
        cfg = cfg._replace(commit=commit)
    if tag is not None:
        cfg = cfg._replace(tag=tag)
    if push is not None:
        cfg = cfg._replace(push=push)
    return cfg

[timobrembeck]

Thanks for the quick review!

Can you say something about your thought process for not just doing this?

Sure, I just thought it might be helpful to have a little bit of error handling in case the options are used incorrectly... Since there are also basic checks when parsing the config file, I enforced the same rules for command line options. If you think this adds too much overhead, I can just remove the checks.

I thought, people might get confused when they pass e.g. the --tag-commit flag but nothing happens because commit=False in the config file... at this point, I'd argue it's probably more user friendly to implicitly set commit=True when the --tag-commit is given and automatically set tag=False & push=False when no-commit is used. But this would be different to the current behavior of the config file options and is a bit more complex than the current implementation. So I think I still prefer my suggestion that every command line flag only overrides its corresponding config value and an error is thrown if any non-sensible combination of flags is used...

[mbarkhau]

Overhead isn't the issue. I tried to use the feature and was puzzled that it would not accept only the flag --no-commit, but also wanted --no-tag-commit and --no-push.

I think you are correct, that each flag should only override it's corresponding config option. The cascade I'm speaking of is already implemented based on the config options.

[timobrembeck]

Ah okay, yeah I just tried to replicate the behavior of the config file, when you set commit=False and tag=True, it also throws an error instead of doing nothing. But I also see the advantage of only having to disable the commit option and implicitly deactivate tag & push... this means I would just have to remove the two checks here and here, right?

[mbarkhau]

Yes, I think those checks make the feature cumbersome to use.

[timobrembeck]

Alright, I removed the checks, adapted the test cases and pointed out in the readme that --no-commit also disables tag and push.

[mbarkhau]

I've made some minor changes with f6280cf. If you're happy with them, I'll merge and cut a release.

[timobrembeck]

Sure, looks good! :+1: Thanks!