Skip to content

Universal CLI for all tools#8

Merged
stlehmann merged 22 commits intostlehmann:masterfrom
jrhawley:universal-cli
May 8, 2020
Merged

Universal CLI for all tools#8
stlehmann merged 22 commits intostlehmann:masterfrom
jrhawley:universal-cli

Conversation

@jrhawley
Copy link
Copy Markdown
Contributor

Hi @stlehmann,

I don't know if you're still actively maintaining this repo or not, but I recently stumbled upon it and think it's great and it has all the nice functionality I've wanted in a command line tool for manipulating PDFs.

What I wanted to have was a universal CLI command to call, with each tools as a sub-command, instead of having pdfmerge.py, pdfsplit.py, etc in my PATH and calling those commands.

So in this PR, I've refactored the code to have a single entry point (pdftools), with all the appropriate functions as sub-commands (e.g. pdftools merge).
I've also formatted some of the arguments to have consistent naming and help statements to try and help with clarity.

If you don't want to merge this PR that's cool, I don't mind. I figured if I refactored this code to make the tool more accessible for myself, then you/others might be interested in it too.

@stlehmann
Copy link
Copy Markdown
Owner

Hey James, thank you for your PR. This is one of my first open-source repositiories and as you could see I haven't touched it for a long time. However, I would like to keep it up-to-date. I like your idea of using one common entrypoint for all commands. I will have a look at your proposed changes and review your PR asap.

Copy link
Copy Markdown
Owner

@stlehmann stlehmann left a comment

Choose a reason for hiding this comment

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

Looks very good to me. Of course the README.md needs to be rewritten, too :).

Comment thread pdftools/_cli.py
help="Merge the pages of multiple input files into one output file",
formatter_class=argparse.ArgumentDefaultsHelpFormatter,
)
parser_merge.add_argument(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

For all other commands the output file is defined by the -o/--output argument. This should be the same here, too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I think I left it as a required argument because you need an output file name to write to, but adding a default, as I've done in the most recent commit, should take care of this

@jrhawley
Copy link
Copy Markdown
Contributor Author

jrhawley commented May 8, 2020

I just pushed a few commits to address your earlier points, but it looks like the TravisCI checks are failing. I looked into them and it looks like there's an issue with Travis setting up Python 3.2 and 3.3 environments. v3.4 - nightly appear to be passing, though

@stlehmann
Copy link
Copy Markdown
Owner

That looks great. Thanks lots for your effort here. It think these changes demand for a new mayor version. I'll merge this and take care of the rest.

@stlehmann stlehmann merged commit 46d7381 into stlehmann:master May 8, 2020
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.

2 participants