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

feat: add IsFalseLike #23

Merged
merged 6 commits into from
Mar 29, 2022
Merged

Conversation

osintalex
Copy link
Contributor

I thought this library looked pretty cool so wanted to give this a go.

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #23 (f287e1c) into main (6e42e04) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #23   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9        10    +1     
  Lines          584       599   +15     
  Branches       153       155    +2     
=========================================
+ Hits           584       599   +15     
Impacted Files Coverage Δ
dirty_equals/_base.py 100.00% <ø> (ø)
dirty_equals/__init__.py 100.00% <100.00%> (ø)
dirty_equals/_boolean.py 100.00% <100.00%> (ø)

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 6e42e04...f287e1c. Read the comment docs.

Check if the value is False like, and matches the given conditions.
"""

allowed_types: Union[Type[B], Tuple[Union[_SpecialForm, type], ...]] = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be a much simpler way to do this, I got quite confused working with _SpecialForm and mypy here. This also requires a type ignore here.

@osintalex osintalex marked this pull request as ready for review March 8, 2022 13:47
@osintalex
Copy link
Contributor Author

Hey, just wondering if there's any interest on this? Happy to try and help with something else if that would be a better fit, I think it's a really cool project!

@samuelcolvin
Copy link
Owner

Sorry, somehow I missed this. I'll look soon.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

This is looking great, just a few things to fix.

We'll need to add IsTrueLike in the same release, but I can do that.

Check if the value is False like, and matches the given conditions.
"""

allowed_types: Union[Type[B], Tuple[Union[_SpecialForm, type], ...]] = (
Copy link
Owner

@samuelcolvin samuelcolvin Mar 25, 2022

Choose a reason for hiding this comment

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

I think we should allow all types to IsFalseLike, with that:

  • the logic will be simpler - effectively return not bool(thing) (except for the string logic)
  • it will be more pythonic
  • it will be easier for users to understand - they don't have to remember (or look up) what types allowed - they can just use their intuition about python


def __init__(
self,
numeric: Optional[bool] = None,
Copy link
Owner

Choose a reason for hiding this comment

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

this is not required:

assert bool(1) is True
assert bool(0) is False

Is already the case.

def __init__(
self,
numeric: Optional[bool] = None,
string: Optional[bool] = None,
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is good, but we should ignore case and give it a more descritive name.

assert 'false' == IsFalseLike(allow_strings=True)

also, I think you should make it a keyword only argument.

if self.numeric and not isinstance(self.numeric, bool):
raise ValueError('"numeric" requires a boolean argument')
if self.string and not isinstance(self.string, bool):
raise ValueError('"string" requires a boolean argument')
Copy link
Owner

Choose a reason for hiding this comment

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

no need for explicit type checks like this, we don't do it elsewhere.

@@ -0,0 +1,3 @@
# Boolean Types
Copy link
Owner

Choose a reason for hiding this comment

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

good.



@pytest.mark.parametrize(
'other, expected',
Copy link
Owner

Choose a reason for hiding this comment

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

good.

@osintalex
Copy link
Contributor Author

Thanks for the helpful feedback! All makes sense, about to push another commit now.

@osintalex
Copy link
Contributor Author

I think those check fails are all about an error in the test_is_now_tz test intest_datetime.py tests. Plus this jinja update which breaks mkdocs. Looks like the latest version of mkdocs has fixed it though.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Otherwise looking good, main should now be fixed, so you can rebase to fix build

Comment on lines 35 to 37
super().__init__(
allow_strings=allow_strings,
)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
super().__init__(
allow_strings=allow_strings,
)
super().__init__(allow_strings=allow_strings)


assert False == IsFalseLike
assert 0 == IsFalseLike
assert '0' == IsFalseLike(allow_strings=True)
Copy link
Owner

Choose a reason for hiding this comment

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

put the assert 'false' == IsFalseLike(allow_strings=True) case before these.

You should also add assert 'foobar' != IsFalseLike(allow_strings=True) to show that other values don't raise an error.

allow_strings: Optional[bool] = None,
):
"""
Example of basic usage:
Copy link
Owner

Choose a reason for hiding this comment

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

you need to add Args and document what values are allowed when allow_strings=True.


class IsFalseLike(DirtyEquals[B]):
"""
Check if the value is False like, and matches the given conditions.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Check if the value is False like, and matches the given conditions.
Check if the value is False like, `IsFalseLike` allows comparison to anything and effectively uses
`return not bool(other)` (with string checks if `allow_strings=True` is set).
Or similar

Comment on lines 46 to 49
if other.lower() in ('0', '0.0', 'false'):
return True
else:
return False
Copy link
Owner

Choose a reason for hiding this comment

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

You can use just:

Suggested change
if other.lower() in ('0', '0.0', 'false'):
return True
else:
return False
return other.lower() in {'0', '0.0', 'false'}

in checks are almost always faster with sets than tuples or lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! Did not know that re sets.

)

def equals(self, other: Any) -> bool:
if self.allow_strings:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if self.allow_strings:
if isinstance(other, str) and self.allow_strings:

Otherwise you'd get an error here!

Comment on lines 37 to 39
def test_dirty_not_equals(self, other, expected):
with pytest.raises(AssertionError):
assert other != expected
Copy link
Owner

Choose a reason for hiding this comment

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

this isn't achieving anything, you can remove it and make this a simple function test.

@samuelcolvin samuelcolvin merged commit 7a02b01 into samuelcolvin:main Mar 29, 2022
@samuelcolvin
Copy link
Owner

thanks so much.

For future reference, you've done something weird here when updating from main, which means my changes are showing in your PR, but shouldn't be a problem on this occation.

@samuelcolvin samuelcolvin mentioned this pull request Mar 29, 2022
20 tasks
@osintalex
Copy link
Contributor Author

Pleasure - thanks for the feedback. My bad on that, think I should have forced push but instead hit git pull origin <branch> --rebase. Will beard in mind for the future 👍🏻

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