-
-
Notifications
You must be signed in to change notification settings - Fork 410
Support optional values through a new validator. #17
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
Conversation
Unfortunately, I'm unable to run tests locally b/c of severe dependency hell. I am unable to get tox, detox, python setup.py test, and other means of running tests to work at all. So, since I'm blocked on a project by the lack of optional validator support, I am going to close my eyes, look away, and pull the trigger, and hope I hit the target. If someone who has a working Python configuration can please be kind enough to let me know if my changes fail any tests, I'd be very appreciative. Even better, I'd love to know why a stock Python distribution with dependencies installed is incapable of running the tests for attrs. However, this isn't the right forum to answer that question. (This PR has the 17+ debugging commits cleaned up.)
Current coverage is
|
|
As for your dependency hell, the answer is to use virtualenvs for everything: https://hynek.me/articles/virtualenv-lives/ For CLI tools I like to use pipsi that will install them into separate virtualenvs (that’s where I have tox in). To run the tests you only should require tox and run it. But as you see, your changes passed in Travis so there’s that. :) The idea makes sense to me, a few comments:
The rest will follow inline, thank you for your contribution! |
attr/validators.py
Outdated
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
docs/changelog.rst
Outdated
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
I was wondering if you'd had a chance to re-review this patch? This is a project blocker for me. Thanks! |
|
I'm sorry I haven't got a notification about your changes so please ping a PR if you address a review. Right now at a conference but I'll look at it when I'm back. |
|
Merged in b54bbaa – thanks! |
Unfortunately, I'm unable to run tests locally b/c of severe dependency
hell. I am unable to get tox, detox, python setup.py test, and other
means of running tests to work at all. So, since I'm blocked on a
project by the lack of optional validator support, I am going to close
my eyes, look away, and pull the trigger, and hope I hit the target.
If someone who has a working Python configuration can please be kind
enough to let me know if my changes fail any tests, I'd be very
appreciative.
Even better, I'd love to know why a stock Python distribution with
dependencies installed is incapable of running the tests for attrs.
However, this isn't the right forum to answer that question.
(This PR has the 17+ debugging commits cleaned up.)