Skip to content

Conversation

@gpshead
Copy link

@gpshead gpshead commented May 20, 2019

This fixes micropython#2287.

@dhalbert
Copy link
Collaborator

I'm not sure: did you want to submit this to circuitpython or micropython?

Looking at https://docs.python.org/3/library/binascii.html#binascii.hexlify, it appears that CPython doesn't take a second argument in any case. We aspire to be the same as "regular" Python or be a subset, not a superset, so just dropping the second argument is something we'd probably do in the long run.

@gpshead
Copy link
Author

gpshead commented May 20, 2019

it's a good question... I'm happy to make a PR removing it instead if you'd like.

I was pondering adding the feature to CPython but realistically, binascii.hexlify is redundant in CPython as bytes, memoryview, and bytearray all have a .hex() method as well as a .fromhex() method on bytes and bytearray - no legacy module function needed. If I were to add it to CPython it'd likely go on those methods.

MicroPython doesn't appear to have those. They would get rid of the need for binascii for hexlify and unhexlify purposes entirely.

This fix if it stays anywhere does make more sense in micropython proper. (i was originally going to send a PR there given where this function comes from but github won't let me fork micropython due to my circuitpython fork existing - apparently i'm supposed to learn crazy git voodoo to branch off of the last micropython sync point or pull a new upstream remote into my circuitpython repo as an alternate master from the micropython repo? i'm not really sure... ugh, guess i need to untangle that)

@tannewt
Copy link
Member

tannewt commented May 20, 2019

+1 To having CircuitPython work the way you'd do it in CPython. (Though it's tricky if you, @gpshead, makes it a moving target. :-P)

Here is the git voodoo I think you'd need:

  1. Create a new branch git checkout -b <new branch name>
  2. In your local CircuitPython repo: git remote add micropython git@github.com:micropython/micropython.git
  3. Then git rebase -i micropython/master. It should show four commits in the prompt (same as the PR). It should rebase just fine because I don't think we've changed those files toooo much. The old branch should be this version still.
  4. Once the commits are updated then push it to git under the new branch. In the web UI GitHub will offer to create a PR. Click it but then it'll show a ton and be based off CircuitPython. At the top you'll need to change the repo to micropython's instead. Then it should just show the four commits and you can PR it.

The critical piece to realize is that circuitpython and micropython are different remote versions of the same code repo. circuitpython/master and micropython/master are just two branches from the same lineage. The "fork" relationship is a GitHub-ism that only manifests as the (sometimes not great) PR default target.

@dhalbert
Copy link
Collaborator

@gpshead If you'd like to make a PR to remove it that's fine. But another thing on the lis to do is to refactor extmod/modubinascii.c into shared-bindings and shared-module, as we've done with some other modules. It would probably make sense to leave extmod/modubinascii.c and remove the second arg as part of refactoring.

I notice we're missing issues for this refactoring, so I'll open individual ones. Added #1899 for binascii.

@dhalbert
Copy link
Collaborator

@gpshead Closing this for now. Let's remove the extra argument to be compatible with CPython.

@dhalbert dhalbert closed this May 28, 2019
@gpshead
Copy link
Author

gpshead commented Jun 14, 2019

fyi - I added sep in CPython 3.8. :)

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.

Surprising behavior for separator argument to ubinascii.hexlify()

3 participants