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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error for in_(a_string) with a non-string value #383

Merged
merged 4 commits into from May 25, 2018

Conversation

Projects
None yet
3 participants
@Zac-HD
Contributor

Zac-HD commented May 23, 2018

  • Added tests for changed code.
  • Changes (and possible deprecations) have news fragments in changelog.d.
  • Is my first attrs PR 馃槏

Closes #382.

@codecov

This comment has been minimized.

codecov bot commented May 23, 2018

Codecov Report

Merging #383 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #383   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         831    835    +4     
  Branches      174    174           
=====================================
+ Hits          831    835    +4
Impacted Files Coverage 螖
src/attr/validators.py 100% <100%> (酶) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 8274c9f...1a28db0. Read the comment docs.

@wsanchez

Please address comments.

if value not in self.options:
try:
in_options = value in self.options
except Exception as e: # e.g. `1 in "abc"`

This comment has been minimized.

@wsanchez

wsanchez May 24, 2018

I think we want TypeError, not Exception here. Other exceptions don't necessarily imply that value isn't one of the items in self.options.

@@ -243,6 +243,18 @@ def test_fail(self):
"'test' must be in [1, 2, 3] (got None)",
) == e.value.args
def test_fail_with_string(self):
"""
Same error for string options as for list options, not TypeError.

This comment has been minimized.

@wsanchez

wsanchez May 24, 2018

Don't like "Same error" referencing鈥 probably the prior test. I'd change to:

Raise ValueError if the value is outside our options when the options are specified as a string and the value is not a string.
@Zac-HD

This comment has been minimized.

Contributor

Zac-HD commented May 24, 2018

@wsanchez - done 馃槃

@@ -0,0 +1,3 @@
``in_()`` validators now raise a ValueError with a useful message

This comment has been minimized.

@hynek

hynek May 25, 2018

Member

It would be faster to fix it myself, but for the next time:

  • We use semantic newlines.
  • We use fully qualified names for functions (e.g. attr.validators.in_()) which simplifies the sentences 鈥 鈥attr.validators.in_() now raises鈥︹.
  • 鈥渞aises an unhelpful TypeError鈥 makes it sound like it鈥檚 present behavior which it鈥檚 not. :)
  • Please wrap symbols like ValueError or TypeError in double ticks.

Yeah, sorry I care about changelogs. 馃槆

This comment has been minimized.

@Zac-HD

Zac-HD May 25, 2018

Contributor

No worries, I care about them too! Fixed 鉂わ笍

@@ -0,0 +1,2 @@
``attr.validators.in_()`` now raises a ``ValueError`` with a useful message even if the options are a string and the value is not a string.
Previously, this raised a ``TypeError`` (matching e.g. ``1 in "abc"``).

This comment has been minimized.

@hynek

hynek May 25, 2018

Member

I cannot quite parse the 鈥渕atching鈥 part 鈥 any chance to rephrase that? I genuinely don鈥檛 know what that means and it almost looks like a typo to me. :)

This comment has been minimized.

@Zac-HD

Zac-HD May 25, 2018

Contributor

How about "(just like 1 in "abc")" ?

This comment has been minimized.

@hynek

hynek May 25, 2018

Member

ah yeah maybe drop the just and we鈥檙e golden

@hynek

hynek approved these changes May 25, 2018

@hynek hynek merged commit a3dbdfc into python-attrs:master May 25, 2018

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 8274c9f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hynek

This comment has been minimized.

Member

hynek commented May 25, 2018

Thanks!

@Zac-HD Zac-HD deleted the Zac-HD:in-string branch May 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment