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

Add is_hexstr function #183

Merged
merged 13 commits into from Jan 14, 2020

Conversation

adamjsawicki
Copy link
Contributor

@adamjsawicki adamjsawicki commented Dec 27, 2019

What was wrong?

Fixes #137, it was decided that a second function is_hexstr should be added that returns False on any
non-string input call to the function.

How was it fixed?

The specified function was implemented.

To-Do

  • [ X] Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

Adam Sawicki added 5 commits December 27, 2019 16:22
As per ethereum#137,
it was decided that a new function should be added with
the signature above that returns 'False' on any non-string
input.

The function has been added with appropiate test cases.
Due to the implementation of `is_hexstr()`, calls
to `is_text` are unneeded that occur before any call
to `is_hexstr()`.
@adamjsawicki adamjsawicki changed the title Add is hexstr function Add is_hexstr function Dec 27, 2019
Copy link
Contributor

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

🔥 Thanks for taking this on @swixx ! Just a couple thoughts and then we're good to merge!

  • It can be helpful in the pr descriptions to put fixes #183 instead of per #183 since with fixes it will automatically close the issue once this is merged.
  • If you could update the docs/utilities.rst file to match what's been done in this pr, that'd be great! (eg. note that is_hex is deprecated, and update the is_hex docs to is_hexstr)

newsfragments/137.feature.rst Outdated Show resolved Hide resolved
tests/hexadecimal-utils/test_is_hexstr.py Outdated Show resolved Hide resolved
warnings.warn(
DeprecationWarning(
"is_hex(value: Any) has been deprecated and will be removed in a subsequent major version "
"release of the eth-utils library. Update your calls to use is_hexstr(Any) instead."
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: for consistency with the beginning of the warning msg consider updating to is_hexstr(value: Any)

eth_utils/hexadecimal.py Outdated Show resolved Hide resolved
@carver
Copy link
Collaborator

carver commented Jan 6, 2020

  • eg. note that is_hex is deprecated

Let's separate that part out into another issue. I think we might want to keep both versions, where is_hex is the stricter option and presumes str. If @swixx opens another issue for that, then the discussion about keeping it doesn't need to block this PR.

@adamjsawicki
Copy link
Contributor Author

@njgheorghita Made the requested changes. Per @carver's, I did not add anything to docs/utilities.rst.

("0xabcdef1234567890", True),
("0xABCDEF1234567890", True),
("0xAbCdEf1234567890", True),
("0XAbCdEf1234567890", True),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this line got duplicated

@njgheorghita
Copy link
Contributor

njgheorghita commented Jan 9, 2020

Let's separate that part out into another issue. I think we might want to keep both versions, where is_hex is the stricter option and presumes str.

There's maybe some confusion around this from @carver's comment, at least there is on my end. I understood "that part" to reference the deprecation of is_hex() in the code - and not the task of updating the docs. eg. We want users to still be able to use is_hex() without seeing a deprecation warning.

If correct, there's a couple last changes - thanks again @swixx , sorry about the endless nitpicks.

  • remove the deprecation warnings from is_hex() and its corresponding tests
  • update docs/utilities.rst to document is_hexstr() and note why it's different from is_hex() - regardless of whether or not we deprecate is_hex() here, is_hexstr() should be documented

Finally, if you're up for it @swixx ...

If @swixx opens another issue for that, then the discussion about keeping it doesn't need to block this PR.

Where you should deprecate is_hex() and its corresponding tests/docs

@carver
Copy link
Collaborator

carver commented Jan 9, 2020

Yup, thanks for clarifying @njgheorghita -- that's exactly what I meant.

@adamjsawicki
Copy link
Contributor Author

@carver will submit tonight. Just so we don't have to do another back and forth, how's the following?

.. note:: 

    This function differs from ``is_hex(value: Any)`` in that it will return
    False on all non-text type arguments, while ``is_hex`` will raise a ``TypeError``
    for all non-text type arguments.

@carver
Copy link
Collaborator

carver commented Jan 13, 2020

@carver will submit tonight. Just so we don't have to do another back and forth, how's the following?

.. note:: 

    This function differs from ``is_hex(value: Any)`` in that it will return
    False on all non-text type arguments, while ``is_hex`` will raise a ``TypeError``
    for all non-text type arguments.

Yup, looks good!

docs/utilities.rst Outdated Show resolved Hide resolved
Co-Authored-By: Jason Carver <ut96caarrs@snkmail.com>
@carver
Copy link
Collaborator

carver commented Jan 13, 2020

@njgheorghita after you do your last lookover, no need to wait for me to merge 👍

Copy link
Contributor

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @swixx !!

@njgheorghita njgheorghita merged commit 8d9f2d7 into ethereum:master Jan 14, 2020
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.

Should is_hex() return False if the argument is not a str?
3 participants