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

Add type validation. #2097

Merged
merged 1 commit into from
Nov 28, 2016
Merged

Add type validation. #2097

merged 1 commit into from
Nov 28, 2016

Conversation

decentral1se
Copy link
Contributor

@decentral1se decentral1se commented Nov 28, 2016

Changes to add to the conversation in #2089.

Something which may not be acceptable is the reduced specificity of the error messages. For example, the validation function filename can't refer to --junit-xml in the error message.

Let me know what you think @nicoddemus.

@nicoddemus
Copy link
Member

Looks good so far!

Something which may not be acceptable is the reduced specificity of the error messages.

How about making filename and directory receive an extra parameter indicating the name of the option, and then we bind to the actual option name using functools.partial at declaration time?

Something like:

def filename(optname, path):
    """Argparse type validator for filename arguments"""
    if os.path.isdir(path):
        raise UsageError("Must be a filename, given: {0}".format(path))
    return path

And when registering the option, instead of:

type=filename,

We use:

type=functools.partial(optname='--junitxml'),

@@ -70,6 +70,18 @@ class UsageError(Exception):
""" error in pytest usage or invocation"""


def filename(path):
Copy link
Member

Choose a reason for hiding this comment

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

How about naming this something which makes it more explicit that it is a argparse type validator? How about filename_arg or something like this?

Copy link
Contributor Author

@decentral1se decentral1se Nov 28, 2016

Choose a reason for hiding this comment

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

👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 92.826% when pulling 733d719 on lwm:junitxml-warn into 0735d45 on pytest-dev:master.

@decentral1se
Copy link
Contributor Author

Oh, functools, my favourite. Here comes a fixup!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 92.827% when pulling 86a65c5 on lwm:junitxml-warn into 0735d45 on pytest-dev:master.

@@ -1,6 +1,9 @@
3.0.5.dev0
==========

* Add Argparse style type validation for ``--confcutdir`` and ``--junit-xml`` (`#2089`_).
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps something more user oriented, for example:

* Now ``--confcutdir`` and ``--junit-xml`` are properly validated if they are directories and filenames, respectively (`#2089`_ and `#2078`_). Thanks to `@lwm`_ for the PR.

With this, you can also remove the entry "An error message is now displayed if --confcutdir is not a valid directory..." some items below this one, since it replaces it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 92.827% when pulling 6cc6894 on lwm:junitxml-warn into 0735d45 on pytest-dev:master.

Argparse driven argument type validation is added for the
`--junit-xml` and `--confcutdir` arguments.

The commit partially reverts #2080. Closes #2089.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 92.827% when pulling 6cc6894 on lwm:junitxml-warn into 0735d45 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 92.827% when pulling 4e1609b on lwm:junitxml-warn into 0735d45 on pytest-dev:master.

@nicoddemus
Copy link
Member

Thanks!

@decentral1se
Copy link
Contributor Author

🚀

@nicoddemus
Copy link
Member

Unless someone else wants more time to review this, I plan to merge this at the end of the day. 😁

@@ -70,6 +70,28 @@ class UsageError(Exception):
""" error in pytest usage or invocation"""


def filename_arg(path, optname):
""" Argparse type validator for filename arguments.
Copy link
Contributor

@blueyed blueyed Nov 28, 2016

Choose a reason for hiding this comment

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

Extra space here.. s/""" A/"""A/.

But also done like this elsewhere. So let's just keep it.

@nicoddemus nicoddemus merged commit 454d288 into pytest-dev:master Nov 28, 2016
@blueyed
Copy link
Contributor

blueyed commented Nov 28, 2016

@lwm
Thanks! Welcome to the team! ;)

@nicoddemus
Copy link
Member

Thanks! Welcome to the team! ;)

Ditto. 😁 👍

@decentral1se decentral1se deleted the junitxml-warn branch November 29, 2016 10:05
@decentral1se
Copy link
Contributor Author

😄 🍷

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.

5 participants