Skip to content

test: refactor: remove hex_str_to_bytes helper #22593

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
Aug 2, 2021
Merged

test: refactor: remove hex_str_to_bytes helper #22593

maflcko merged 1 commit into from
Aug 2, 2021

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Jul 31, 2021

Use the built-in class method bytes.fromhex() instead, which is available since Python 3.0 (https://docs.python.org/3.0/library/stdtypes.html?highlight=fromhex#bytes.fromhex).

This would be nice to solve with a scripted-diff, but it's tricky to also tackle the imports (which need to be removed in the same commit, otherwise the linter would complain).

@bitcoin bitcoin deleted a comment from Dunlap28 Jul 31, 2021
@DrahtBot DrahtBot added the Tests label Jul 31, 2021
@practicalswift
Copy link
Contributor

practicalswift commented Jul 31, 2021

Concept ACK

Zero-1729
Zero-1729 approved these changes Jul 31, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

Concept and code review ACK ee54626838f014efa9cc858df28e717327075565

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 1, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Aug 1, 2021
Merged
@theStack
test: refactor: remove hex_str_to_bytes helper
ca6c154 
Use the built-in class method bytes.fromhex() instead,
which is available since Python 3.0.
@theStack theStack force-pushed the 202107-test-remove_unneeded_hexstrtobytes branch from ee54626 to ca6c154 Compare August 1, 2021 17:30
@theStack
Copy link
Contributor Author

theStack commented Aug 1, 2021

Rebased on master due to conflict after merge of #22429.

For the previous ACK (thanks Zero-1729!), it can be trivially re-reviewed via git range-diff ee5462683...ca6c154ef

Zero-1729
Zero-1729 approved these changes Aug 1, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

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

re-ACK ca6c154, clean rebase.

@practicalswift
Copy link
Contributor

practicalswift commented Aug 1, 2021

cr ACK ca6c154

@tryphe
Copy link
Contributor

tryphe commented Aug 2, 2021 •
edited
Loading

Concept ACK, cr ACK ca6c154

Silly question, shouldn't we also test bytes.fromhex()? Or maybe not if there's enough constants checked in other places.

@maflcko maflcko merged commit bb60960 into bitcoin:master Aug 2, 2021
@laanwj
Copy link
Member

laanwj commented Aug 2, 2021

FWIW unhexlify can take str as well. There was already no need for the encode(). Though I guess using a built-in is slightly better.

@maflcko
Copy link
Member

maflcko commented Aug 2, 2021

I slightly prefer the clear bytes.fromhex name over the cryptic a2b_hex or the negation unhexlify. In any case it could make sense to remove the a2b_hex and unhexlify uses. It doesn't really make sense to use four different ways (three after this pull), sometimes even mixed within the same file, to achieve the same outcome.

@theStack theStack deleted the 202107-test-remove_unneeded_hexstrtobytes branch August 2, 2021 14:13
@theStack theStack mentioned this pull request Aug 2, 2021
Closed
@Zero-1729 Zero-1729 mentioned this pull request Aug 3, 2021
Closed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 4, 2021
Merge bitcoin#22593: test: refactor: remove hex_str_to_bytes helper
0ca1dd9
maflcko pushed a commit that referenced this pull request Aug 5, 2021
Merge #22619: test: refactor: use consistent bytes <-> hex-string con…
f4328eb 
…version in functional test framework

5a1bef6 test: refactor: remove binascii from test_framework (Zero-1729)

Pull request description:

  This PR continues the work started in PR #22593, regarding using the `bytes` built-in module. In this PR specifically, instances of `binascii`'s methods `hexlify`, `unhexlify`,  and `a2b_hex` have been replaced with the build-in `bytes` module's `hex` and `fromhex` methods where appropriate to make bytes <-> hex-string conversions consistent across the functional test files and test_framework.

  Additionally, certain changes made are based on the following assumption:

  ```
  bytes.hex(data) == binascii.hexlify(data).decode()
  bytes.hex(data).encode() == binascii.hexlify(data)
  ```

  Ran the functional tests to ensure behaviour is still consistent and changes didn't break existing tests.

  closes #22605

ACKs for top commit:
  theStack:
    Code-review ACK 5a1bef6 🔢

Tree-SHA512: 8f28076cf0580a0d02a156f3e1e94c9badd3d41c3fbdfb2b87cd8a761dde2c94faa5f4c448d6747b1ccc9111c3ef1a1d7b42a11c806b241fa0410b7529e2445f
@laanwj laanwj mentioned this pull request Aug 16, 2021
Merged
maflcko pushed a commit that referenced this pull request Aug 21, 2021
Merge #22633: refactor: Replace remaining binascii method calls
f5a406f 
021daed refactor: replace remaining binascii method calls (Zero-1729)

Pull request description:

  This PR removes the remaining `binascii` method calls outside `test/functional` and `test_framework`, as pointed out here  #22619 (review).

  Follow-up to #22593 and #22619
  Closes #22605

ACKs for top commit:
  josibake:
    re-ACK 021daed
  theStack:
    re-ACK 021daed

Tree-SHA512: 2ae9fee8917112c91a5406f219ca70f24cd8902b903db5a61fc2de85ad640d669a772f5c05970be0fcee6ef1cdd32fae2ca5d1ec6dc9798b43352c8160ddde6f
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 7, 2022
@PiRK
test: refactor: remove hex_str_to_bytes helper
f19bc27 
Summary:
Use the built-in class method `bytes.fromhex()` instead, which is available since Python 3.0.

This is a backport of [[bitcoin/bitcoin#22593 | core#22593]]

Test Plan: `ninja check-functional-extended`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11136
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Zero-1729 Zero-1729 Zero-1729 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@theStack @practicalswift @DrahtBot @tryphe @laanwj @maflcko @Zero-1729