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

Deprecate float sizes (min_size, max_size) #1635

Merged
merged 14 commits into from Oct 11, 2018

Conversation

math2001
Copy link
Contributor

@math2001 math2001 commented Oct 11, 2018

Closes #1619

It shows a deprecation warning when min_size or max_size is a float, instead of being an integer.

The tests only target "whole" float number like 5.0, and not 5.1.

I tried to find somewhere in the documentation where to mention this deprecation, but I didn't find anything... I'm not really sure if it'd be that useful anyway...

It also changes the type hints for average_size to being None instead of int.

Lastly, this broke a test with NaN values: a test (in cover/test_direct_strategies.py, at test_validates_keyword_arguments) checks a bunch of invalid arguments and asserts that they raise InvalidArgument.

One of them tests min_size as float('nan'). And this breaks because it raises a warning that it doesn't expect (the warning that this PR adds). Here's the error message

Traceback (most recent call last):                                                                                                                                                                
  File "/home/math2001/code/python/hypothesis/hypothesis-python/tests/cover/test_direct_strategies.py", line 158, in test_validates_keyword_arguments                                             
    fn(**kwargs).example()                                                                                                                                                                        
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/searchstrategy/strategies.py", line 304, in example                                                                
    phases=tuple(set(Phase) - {Phase.shrink}),                                                                                                                                                    
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/core.py", line 985, in find                                                                                        
    specifier.validate()                                                                                                                                                                          
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/searchstrategy/strategies.py", line 379, in validate                                                               
    self.do_validate()                                                                                                                                                                            
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/searchstrategy/lazy.py", line 127, in do_validate                                                                  
    w = self.wrapped_strategy                                                                                                                                                                     
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/searchstrategy/lazy.py", line 113, in wrapped_strategy                                                             
    *self.__args, **self.__kwargs                                                                                                                                                                 
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/strategies.py", line 631, in lists                                                                                 
    check_valid_sizes(min_size, average_size, max_size)                                                                                                                                           
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/internal/validation.py", line 156, in check_valid_sizes                                                            
    check_valid_size(min_size, 'min_size')                                                                                                                                                        
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/internal/validation.py", line 119, in check_valid_size                                                             
    name, value                                                                                                                                                                                   
  File "/home/math2001/code/python/hypothesis/hypothesis-python/src/hypothesis/_settings.py", line 873, in note_deprecation                                                                       
    warnings.warn(warning, stacklevel=2)                                                                                                                                                          
hypothesis.errors.HypothesisDeprecationWarning: Float size are deprecated: min_size should be an integer, got nan

So, I tried to find a way for this test to ignore the warning, without breaking all the other tests in the table.

I've tried to use @pytest.mark.filterwarnings, without success, but weirdly, a with pytest.warns clause works. I'd expect it would make sure every call would raise a warning, but instead, it seems that it just filters them out.

If you want to try it locally:

git remote add mathieu https://github.com/math2001/hypothesis
git fetch mathieu deprecate-float-sizes
git checkout deprecate-float-sizes
pytest "hypothesis-python/tests/cover/test_direct_strategies.py::test_validates_keyword_arguments" 

min_size and max_size should be integers. And we only test whole float numbers,
like 5.0 for example.
float size warning were breaking a test that was making sure that
invalid arguments raised an InvalidArgument error.
lint and check-format tasks on travis
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Solid start - see comments below.

Also make sure that you mention the change of the type hints in RELEASE.rst; something like

The type hint for average_size arguments has been changed from Optional[int] to None, because non-None values are always ignored and deprecated.

I've also just done a quick check for other uses of average_size (with git grep -n "average_size="), and you could delete the use of it in docs/examples.rst. There are plenty more, but all of those uses are OK for compatibility or internal use.

hypothesis-python/tests/cover/test_direct_strategies.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/internal/validation.py Outdated Show resolved Hide resolved
'{} should be an integer, got {}'.format(name, value)
)
else:
check_type(integer_types, value)
if value < 0:
raise InvalidArgument(u'Invalid size %s=%r < 0' % (name, value))
if isinstance(value, float) and math.isnan(value):
Copy link
Member

@Zac-HD Zac-HD Oct 11, 2018

Choose a reason for hiding this comment

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

We can move this to the isinstance(value, float) clause above.

Moving it before the deprecation will also fix the failing test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum... I don't know how I didn't see this... 😢 I'll make sure to read the whole function next time 👀

@Zac-HD Zac-HD added the legibility make errors helpful and Hypothesis grokable label Oct 11, 2018
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

LGTM (looks good to me) 🎉

@Zac-HD Zac-HD merged commit 928e575 into HypothesisWorks:master Oct 11, 2018
@Zac-HD
Copy link
Member

Zac-HD commented Oct 11, 2018

And if you're looking for another (!) issue, #1600 would be a good choice - a bit more complex now that you're comfortable with the process and tests, but still fairly well defined and the code should be quite self-contained. After that it's the deep end, at least for Hypothesis issues.

@math2001 math2001 deleted the deprecate-float-sizes branch October 11, 2018 05:52
@math2001
Copy link
Contributor Author

Whooo! Thank you!

I'll have a look at #1600.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants