CLI interface using click

[khardix]

As suggested on the initial review meeting, I went ahead and rewrote the CLI interface to use click.

I attempted to port it as directly as possible. The most significant changes are:

• Drop of the app module (and subsequently the Application class). Their responsibilities were taken by the cli module and its run function. The Application.run logic was dropped altogether -- click itself handles keyboard interrupts and setting of appropriate exit codes.
• Most of the application tests (tests/test_app.py) were dropped as well -- I see no reason why we should test that the click does its job. The only test ported was test_parse_argv_no_options, both as a way to ensure expected default values and as an example of testing a click application.
• Adjusted entry point in setup.py and add __main__ module. This enables the package to be run directly as a module (python -m rpmlb) as well as console script.

[khardix]

Well, tests fail on missing click (even after I added it into the requirements.txt) – what I am missing?

[hroncok]

That's unwanted side effect of caching .tox. After purging the cache, the build passes. Locally, removing .tox before running it workarounds that issue. Maybe there is a tox option that installs the dependencies again? Not sure.

[hroncok]

• Replacing argparse to click. => Feature A

This is definitely desired.

• Moving main logic app.py to cli.py, removing app.py. => Feature B

Let me have a look at that one.

• Changing entry point interface (Adding python -m rpmlb) => Feature C

That one is very pythonic and I like that.

[hroncok]

Here it's highly related. I don't consider it excellent practice, but I don't feel so against it.

[khardix]

For start, let me state that the discussion here is exactly what I was hoping for – recognizing different patterns of development and agreeing on how to do things :)

TL;DR

1. Yes, it is a big chunk, but doing it in parts would require lot of extra (and in the end pointless) work.
2. The entry point is not changed, but added – you should be able to use both ${prefix}/bin/rpmlb and python -m rpmlb now with the same results.

Long version:

• Replacing argparse to click. => Feature A

I agree it is a big PR. However, argparse and click have different philosophies on how to specify the parameters. Mapping click way to existing Application class would be very unwieldy. What is more, after removing the parts that are handled by click itself, I would end with class with only one method – which is considered an anti-pattern (talk, followup). As a personal opinion, I also dislike classes that will only ever have one instance during a run. In the end I went and transformed the Application class into a single function (cli.run).

In conclusion, I would also like smaller PR, but in this case it would mean an unreasonable amount of extra work.

• Moving main logic app.py to cli.py, removing app.py. => Feature B

I really do not care if the module will be called app or cli, but I tend to use cli for "module holding functions defining the command-line interface". Also the name of the function run is really generic, but I did not think of better one yet.

• Changing entry point interface (Adding python -m rpmlb) => Feature C

It is not changing per se – the ${prefix}/bin/rpmlb is still generated on pip install or equivalent (thanks to setup.py entry-points parameter). I merely added the option to run it without installation (provided that all dependencies are in place, of course) with python -m.

[hroncok]

By the way, now you have 8 commits in this PR, so finally you will do rebase and squash from those commits to 1 commit, right?

Not necessarily. In this case, I'd say no.

[junaruga]

Not necessarily. In this case, I'd say no

@hroncok I want to ask you which kind of case is yes, and which kind of case is no for you?

[hroncok]

I want to ask you which kind of case is yes, and which kind of case is no for you?

If the commits bare significant logic by being separate I deem that appropriate. Example:

1. Introduce a new API for better beer experience
2. Drink all beers using the new API, because it tastes better
3. Add several new beers to our collection (because the new API makes that possible)

Or:

1. Rework the structure of internal modules, so we can introduce more functionality
2. Add new internal module

If they don't, I tent to squash. Example:

1. Implement new Feature
2. Add test for Feature
3. Fix typo in Feature
4. Changes of Feature based on review

[junaruga]

@hroncok beer? you mean beer 🍺 ? Or this is not a implicit or metaphor to another meaning, is it? All right now I knew your needs and the way how you achieve it.

I agree with your needs and the result that you want to gain. Though maybe we have a different opinion about the way to achieve it for each other. I guess that perhaps you are downsizing yourself, and lowering your standard, believing that those are good way to achieve your goal.

If you will squash by yourself finally as a possibility, I am ok or fine for that.

Thanks for your explanation.

[hroncok]

I mean beer as in :beer: No metaphor, just an example.

[khardix]

I am worry that after your change, the test coverage rate becomes lower.

Valid point. I will go through the options and try to come up with reasonable tests. Suggestions for what to test for (i.e. "Ensure that all paths are absolute") are welcome.

By the way, now you have 8 commits in this PR, so finally you will do rebase and squash from those commits to 1 commit, right?

I agree with @hroncok here – I feel that squashing the PR to one commit would needlessly throw away the git history.

[khardix]

So, I have pushed few example tests for the CLI. As I have only vague idea what to test for, I would like get early feedback and save us all some time. Any nitpicking appreciated ☺.

[junaruga]

About this PR, I will delegate the review to Miro. I won't review it.

[khardix]

Looking into pytest docs on parametrizing, I can see how it may be done, but it did not give me a clear idea on usage (i.e. how to determine expected result from the parametrized value, apart from the obvious big if-statement (or equivalent) with branch for each possible value).

Any best-practice examples of the usage of the parametrization?

[hroncok]

Any best-practice examples of the usage of the parametrization?

Nothing that I know of. Let me send you a proposal of how would I do it? (Working on it.)

[hroncok]

See https://github.com/khardix/rpm-list-builder/pull/1

[khardix]

Thanks. I merged the changes and will try to write the rest of the tests more like that.

[khardix]

After the last commit, all the test I could think of should be included. If there will be no further issues with them, I will squash the test-related commits into one (which should also fix the hung-up Travis check, hopefully).

@hroncok, can I ask for re-review (at your convenience)?

[hroncok]

I will re-review once rebased to solve the merge conflicts, as discussed on IRC.