microbit-foundation / micropython-microbit-v2

Temporary home for MicroPython for micro:bit v2 as we stablise it before pushing upstream
MIT License
44 stars 25 forks source link

`AudioFrame` deep copy #189

Closed microbit-carlos closed 2 months ago

microbit-carlos commented 5 months ago

I think the last time we discussed this, we covered two options:

a) Copy Constructor: original = AudioFrame(); copy = AudioFrame(original) b) Method: original = AudioFrame(); copy = original.clone()

Option a) is more Pythonic, while option b) might have been already used in the micro:bit API.

I had an action to look at the existing micro:bit API to see if a) and b) has been used before and which one might be more "microbitish".

Apart from that, we already have keyword arguments in AudioFrame(duration, rate) that we can used as positional arguments in the initialiser, and adding a copy constructor might make the readthedocs signature a bit harder to document (we've had multiple questions about characters like * and / in the function signatures). This one is a minor thing, as we can also explain stuff more in the description, but still worth pointing out.

All in all, I think we should go with the AudioFrame.clone() option, but calling it AudioFrame.copy().

microbit-carlos commented 5 months ago

From @dpgeorge in https://github.com/microbit-foundation/micropython-microbit-v2/pull/163#issuecomment-2068519700:

I was also wondering if we should also make it a static function to be able to do new = AudioFrame.copyfrom(old). Otherwise we now we have to figure out a way to match the length of the old instance somehow when creating the new, as we cannot even do something like new = AudioFrame(size=len(old)); new.copyfrom(old).

How about allowing passing an AudioFrame to the AudioFrame constructor? This is how list, dict etc work in Python. (Did we already discuss point?)

microbit-carlos commented 5 months ago

Yes, using a copy constructor is more Pythonic, but taking in consideration we have two instances already of a copy() method I'm more inclined to use that. What do you think?

microbit-carlos commented 5 months ago

Discussed on our call and agreed to implement the copy() method.

microbit-carlos commented 2 months ago

Based on the changes from https://github.com/microbit-foundation/micropython-microbit-v2/issues/205#issuecomment-2221716206, we should add the copy() method to AudioRecording.