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

[MRG+1] Rename isbinarytext function to binary_is_text for clarity #1851

Merged
merged 1 commit into from Mar 31, 2016

Conversation

@nyov
Copy link
Contributor

@nyov nyov commented Mar 6, 2016

See #1389

Since this suggested rename seemed to have positive responses, here is a PR which does just that, to resolve the issue.

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 6, 2016

Current coverage is 83.17%

Merging #1851 into master will decrease coverage by -0.01% as of 219a61c

Powered by Codecov. Updated on successful CI builds.

return any(c in _BINARYCHARS for c in text)
if not isinstance(data, bytes):
raise TypeError("data must be bytes, got '%s'" % type(data).__name__)
return any(c in _BINARYCHARS for c in data)

This comment has been minimized.

@kmike

kmike Mar 7, 2016
Member

docs say this function returns True if there are no control characters, but this function does the opposite

This comment has been minimized.

@nyov

nyov Mar 7, 2016
Author Contributor

So I actually forgot to change the function body and test... facepalm
Thanks for the thorough look.

@@ -73,16 +73,16 @@ def noncached(self):

class IsBinaryTextTest(unittest.TestCase):

This comment has been minimized.

@kmike

kmike Mar 7, 2016
Member

It is better to rename the test case as well.

@nyov nyov force-pushed the nyov:binary_or_text branch 2 times, most recently from 7e86bf5 to 99c48b5 Mar 7, 2016
@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 7, 2016

Okay I think I got it now.

Let me know if there are any other issues or nitpicks (I don't usually run any pep8 style checker).

"""Return True if the given text is considered binary, or False
otherwise, by looking for binary bytes at their chars
""" This function has been removed.
Please use scrapy.utils.python.binary_is_text """

This comment has been minimized.

@kmike

kmike Mar 7, 2016
Member

I wonder if we should note binary_is_text does the opposite of isbinarytext in a docstring. On one hand, this is easy to check in source code; on the other hand, function names look too similar, it is easy to make a mistake and think the function was renamed.

This comment has been minimized.

@nyov

nyov Mar 8, 2016
Author Contributor

I had considered it. But I wasn't sure if the old function doc string would get checked by anyone (unless checking the source and then it should be obvious), and adding a note to the new function doc about an unrelated function would seem strange as well.

I'll add a note to the old one.

@nyov nyov force-pushed the nyov:binary_or_text branch from 99c48b5 to 4a12c00 Mar 8, 2016
@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 8, 2016

I'll add a note to the old one.

Hope that doc string text makes sense. I'll take anything that might sound better.

""" This function has been removed.
Please use scrapy.utils.python.binary_is_text, which was created to be more
clear about the functions behavior: it is behaving inverted to this one. """
return not binary_is_text(text)

This comment has been minimized.

@nyov

nyov Mar 17, 2016
Author Contributor

@kmike: when you find the time, let me know if this updated docstring fits as a description?
Or anyone else actually, who might feel like replying.

I didn't really find the perfect wording (could sound less ugly), any alternatives welcome.

This comment has been minimized.

@lopuhin

lopuhin Mar 17, 2016
Member

@nyov I like the docstring. Just one nitpick: perhaps it's better to change "This function has been removed" into "This function is deprecated", since it is not removed yet.

This comment has been minimized.

@nyov

nyov Mar 17, 2016
Author Contributor

Indeed, I'll reword that. Thank you

@nyov nyov force-pushed the nyov:binary_or_text branch from 4a12c00 to ebf0efc Mar 17, 2016
@kmike kmike changed the title Rename isbinarytext function to binary_is_text for clarity [MRG+1] Rename isbinarytext function to binary_is_text for clarity Mar 17, 2016
@nyov nyov force-pushed the nyov:binary_or_text branch from ebf0efc to e8ca467 Mar 30, 2016
@redapple redapple merged commit 3ba5671 into scrapy:master Mar 31, 2016
1 of 2 checks passed
1 of 2 checks passed
codecov/patch 63.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nyov nyov deleted the nyov:binary_or_text branch Apr 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants