Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.43%. Comparing base (9670666) to head (1897fe6).
Report is 4 commits behind head on master.

@@            Coverage Diff             @@
##           master   #14387      +/-   ##
==========================================
- Coverage   98.43%   98.43%   -0.01%     
==========================================
  Files         163      163              
  Lines       21295    21294       -1     
==========================================
- Hits        20961    20960       -1     
  Misses        334      334              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code size report:

   bare-arm:   +16 +0.028% 
minimal x86:   +32 +0.017% 
   unix x64:   +32 +0.004% standard
      stm32:    +8 +0.002% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +16 +0.002% RPI_PICO_W
       samd:    +8 +0.003% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Hi @AmirHmZz,

Thanks for noticing this and submitting this PR.

According to CPython documentation, byteorder argument is optional and set to "big" by default. This PR makes Micropython compatible with CPython in this case.

To expand a bit on this, these byteorder arguments only became optional in CPython 3.11 (3.10 version is here). MicroPython only advertises full compatibility with CPython 3.4 at this point.

That said, there are a number of improvements from later CPython versions that are supported in MicroPython and this one might be useful to have. 😁

I was going to suggest adding unit test cases for these as well, although it will be a little fiddly because the unit tests compare behaviour with CPython by default, and only some CPython versions will accept the optional argument.

@projectgus One of the most important points about making that arg optional is reducing redundant allocations and deallocations on "big" string which is used in most cases. I use int>bytes and bytes>int conversions many many times in my projects and most of them are in the loops.

I was going to suggest adding unit test cases for these as well, although it will be a little fiddly because the unit tests compare behaviour with CPython by default, and only some CPython versions will accept the optional argument.

Where should I add unit tests? I will be glad to contribute!

I asked @dpgeorge about this change and it seems like it really will depend on the code size hit as to whether it's desirable to merge. It's currently small, which is encouraging.

However, Damien noted that in Python 3.11 the int.to_bytes() function also gained a default value for the length argument. What's the size impact if this is added as well? It's desirable to keep the CPython compatibility changes consistent as possible, although if that additional change somehow has much higher code size impact then it can be dropped.

Where should I add unit tests? I will be glad to contribute!

Thanks, that'd be great!

tests/basic/int_bytes.py has the current test cases for int.to_bytes and int.from_bytes. However, this test file works by comparing output to CPython which won't be consistent for this change. You'll need to do something like tests/basics/string_format_cp310.py and tests/basics/string_format_cp310.exp which has the test case and expected output in separate files.

Suggest something like int_bytes_optional_args_cp311.py.

making that arg optional is reducing redundant allocations and deallocations on "big" string which is used in most cases. I use int>bytes and bytes>int conversions many many times in my projects and most of them are in the loops.

I would have expected if you're either freezing your .py files into the firmware, or compiling to .mpy with mpy-cross, then the "big" string should end up interned as a QSTR with minimal performance impact. If you've got some "real world" type code where this change makes a significant performance difference, please share it as it'd be interesting to look into.

@projectgus Thanks for feedback. I've added tests and also made length optional in int.to_bytes().

Nice work @AmirHmZz, thanks! Implementation and tests look good to me. It's encouraging this implementation is free or almost free in terms of code size on most of the smaller microcontroller ports.

One thing about the optional length argument for int.to_bytes(), currently MicroPython will truncate to the length if the int doesn't fit (CPython raises an exception.) Which may be a bit counter-intuitive. However, I have a PR open (#13087) which changes the behaviour to match CPython on this.

(Regrettably, that PR adds more than 8 bytes binary size! 😆)

I've rebased this PR against master.

This still looks good to me, and the code size impact is pretty small, but we'll need to wait for @dpgeorge to take a look now.

Thanks for this, it does look good now: CPython compatibility with minimal code size increase.

Rebased and merged!