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

Jump to bottom

bpo-45155 : Default arguments for int.to_bytes(length=1, byteorder=sys.byteorder) #28265

Merged
merged 19 commits into from Sep 16, 2021
Merged

bpo-45155 : Default arguments for int.to_bytes(length=1, byteorder=sys.byteorder) #28265

merged 19 commits into from Sep 16, 2021

Conversation

warsaw
Copy link
Member

@warsaw warsaw commented Sep 9, 2021 •
edited by bedevere-bot

In the PEP 467 discussion, I proposed being able to use

>>> (65).to_bytes()
b'A'

IOW, adding default arguments for the length and byteorder arguments to int.to_bytes()

It occurs to me that this is (1) useful on its own merits; (2) easy to do. So I've done it.

https://bugs.python.org/issue45155

@rhettinger
Copy link
Contributor

rhettinger commented Sep 9, 2021

This is a nice improvement. Thanks.

ericvsmith
ericvsmith reviewed Sep 9, 2021
View changes
Copy link
Member

@ericvsmith ericvsmith left a comment

This all looks reasonable to me. But .format() instead of f-strings? What?!

@rhettinger
Copy link
Contributor

rhettinger commented Sep 9, 2021

The main docs need to be updated as well: https://docs.python.org/3/library/stdtypes.html?highlight=to_bytes#int.to_bytes

@warsaw
Copy link
Member Author

warsaw commented Sep 9, 2021

This all looks reasonable to me. But .format() instead of f-strings? What?!

Ha! I knew you'd say that 😆
I wanted to minimally edit the line so it would fit within my editor line length. That looked like some Python 2.6 code to me!

The main docs need to be updated as well: https://docs.python.org/3/library/stdtypes.html?highlight=to_bytes#int.to_bytes

Yep, thanks @rhettinger ... coming up!

@rhettinger
Copy link
Contributor

rhettinger commented Sep 9, 2021

Also consider updating the default byteorder for int.from_bytes. The to/from round trip should be as symmetrical as possible.

brandtbucher
brandtbucher reviewed Sep 9, 2021
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Beat me to it! I agree that int.from_bytes deserves the same treatment, too.

I also saw one possible improvement:

Objects/longobject.c Outdated Show resolved Hide resolved
JelleZijlstra
JelleZijlstra reviewed Sep 9, 2021
View changes
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
@encukou
Copy link
Member

encukou commented Sep 10, 2021

Please, for the sake of reproducible science (and other cases), don't add platform-dependent defaults.

@warsaw @brandtbucher
Use a faster method to check for endian-ness
cba46c7 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
brandtbucher
brandtbucher requested changes Sep 10, 2021
View changes
Objects/longobject.c Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Core and Builtins/2021-09-09-15-05-17.bpo-45155.JRw9TG.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Sep 10, 2021

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@warsaw
Copy link
Member Author

warsaw commented Sep 10, 2021

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Sep 10, 2021

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from brandtbucher Sep 10, 2021
brandtbucher
brandtbucher approved these changes Sep 10, 2021
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Looks good! Just one RST nit:

Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
warsaw and others added 2 commits Sep 13, 2021
@warsaw @brandtbucher
Fix RST typo
c4c82aa 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw
Default byteorder to 'big'
354576a
@warsaw warsaw requested a review from brandtbucher Sep 15, 2021
@warsaw
Copy link
Member Author

warsaw commented Sep 15, 2021

"big" by default.

brandtbucher
brandtbucher requested changes Sep 15, 2021
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

I think you missed some!

Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Core and Builtins/2021-09-09-15-05-17.bpo-45155.JRw9TG.rst Outdated Show resolved Hide resolved
Objects/longobject.c Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Sep 15, 2021

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

warsaw and others added 5 commits Sep 15, 2021
@warsaw @brandtbucher
Update Doc/library/stdtypes.rst
a321587 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw @brandtbucher
Update Doc/library/stdtypes.rst
d468cac 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw @brandtbucher
Update Doc/library/stdtypes.rst
a6fd648 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw @brandtbucher
Update Doc/library/stdtypes.rst
7eb77dd 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw @brandtbucher
Update Misc/NEWS.d/next/Core and Builtins/2021-09-09-15-05-17.bpo-451…
25ec4bb 
…55.JRw9TG.rst

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw
Copy link
Member Author

warsaw commented Sep 15, 2021

Thanks for catching the ones I missed @brandtbucher

@warsaw
Copy link
Member Author

warsaw commented Sep 15, 2021

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Sep 15, 2021

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from brandtbucher Sep 15, 2021
brandtbucher
brandtbucher approved these changes Sep 15, 2021
View changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Sorry, I promise this is my last review!

Just realized that we can help AC give us a better __text_signature__ (for help, inspect, etc.). Other than that improvement, this looks good!

Objects/longobject.c Outdated Show resolved Hide resolved
Objects/longobject.c Outdated Show resolved Hide resolved
warsaw and others added 2 commits Sep 15, 2021
@warsaw @brandtbucher
Update Objects/longobject.c
3c34b8c 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@warsaw @brandtbucher
Update Objects/longobject.c
b025642 
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@brandtbucher
Copy link
Member

brandtbucher commented Sep 15, 2021

(I think you need to re-run AC locally, too.)

@warsaw warsaw merged commit 07e737d into python:main Sep 16, 2021
12 checks passed
@bedevere-bot
Copy link

bedevere-bot commented Sep 16, 2021

@warsaw: Please replace # with GH- in the commit message next time. Thanks!

@warsaw warsaw deleted the bchr branch Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvsmith ericvsmith

@JelleZijlstra JelleZijlstra

@brandtbucher brandtbucher

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@warsaw @rhettinger @encukou @bedevere-bot @brandtbucher @ericvsmith @JelleZijlstra @the-knights-who-say-ni