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

Minor improvements in CLI #25

Merged
merged 6 commits into from
Jun 19, 2021
Merged

Minor improvements in CLI #25

merged 6 commits into from
Jun 19, 2021

Conversation

cknabs
Copy link
Contributor

@cknabs cknabs commented Jun 13, 2021

This PR adds the following (minor) improvements to the CLI:

  • Removes the unreachable checks for domain and file_types, since this is already done by argparse
  • Generalizes the checks for non-negative arguments
  • Does not create and print the save_directory if download_files is False
  • Allows to specify an output file for saving links when using -f with an argument. If only -f is specified, the links are saved to html_links_<TIMESTAMP>.txt, as before.

Thanks for the good work, really appreciate it!

@opsdisk
Copy link
Owner

opsdisk commented Jun 13, 2021

Thanks for taking the time to submit a PR @cknabs! Please give me a few days to take a look.

help="R|Save the html links to a file.\n"
"no -f = Do not save links\n"
"-f = Save links to html_links_<TIMESTAMP>.txt\n"
"-f SAVE_FILE = Save links to SAVE_FILE",
Copy link
Owner

Choose a reason for hiding this comment

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

Nice upgrade to the -f switch. I like it!

metagoofil.py Outdated
@@ -238,6 +236,16 @@ def _split_lines(self, text, width):
return argparse.HelpFormatter._split_lines(self, text, width)


def pos(type):
def check(value):
Copy link
Owner

Choose a reason for hiding this comment

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

I've never seen this function convention before. It was hard to google for as well...is this called something specifically?

How is the actual value from argparse being passed? Are you overriding a .check() function within argparse or something?

I definitely like the spirit of this and want to keep it, but anyway it could be simplified? Or do you have some documentation recommending this for Python argparse type checking?

Copy link
Contributor Author

@cknabs cknabs Jun 14, 2021

Choose a reason for hiding this comment

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

The type argument of argparse's add_argument() is a callable that takes a string, converts it to the desired type, and can do some basic type-checking (see https://docs.python.org/3/library/argparse.html#type).

Here pos(T) returns such a callable (called check(), but the name has no special meaning) that maps a string to a result of type T. It might be simpler to define separate pos_float() and pos_int(), and then use type=pos_int instead of type=int:

def pos_int(value):
    try:
        value_int = int(value)
        assert value_int >= 0
        return value_int
    except (AssertionError, ValueError):
        raise argparse.ArgumentTypeError(f"invalid value '{value}', must be an int >= 0")

In this particular case, I think checking that these 3 args are positive after parsing is fine, a callable returning a callable / using custom "types" is a bit overkill for such a simple interface.
The improvement provided here is minimal, this kind of custom type is more useful and relevant for a larger number of arguments with similar constraints, which isn't really the case here.

Let me know what you'd prefer, and I can amend the PR :)

Copy link
Owner

Choose a reason for hiding this comment

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

Learn something new every day! If you could make it two separate functions:

  • positive_int()
  • positive_float()

and keep the simple function check you have above instead of a callable returning a callable...that'd be perfect.

(`delay`, `url_timeout`, `number_of_threads`)
@cknabs
Copy link
Contributor Author

cknabs commented Jun 17, 2021

Done, feel free to squash everything into one commit if you think that helps.
Thanks again for the good work, and for taking the time to look at the PR :)

@opsdisk opsdisk merged commit 6e1c45f into opsdisk:master Jun 19, 2021
@opsdisk
Copy link
Owner

opsdisk commented Jun 19, 2021

Thanks for your contributions @cknabs!

@opsdisk opsdisk mentioned this pull request Oct 9, 2022
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