Skip to content

AudioFrame deep copy #189

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

Closed
microbit-carlos opened this issue Apr 9, 2024 · 4 comments
Closed

AudioFrame deep copy #189

microbit-carlos opened this issue Apr 9, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@microbit-carlos
Copy link
Contributor

microbit-carlos commented Apr 9, 2024

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".

  • We currently have two instances of a copy() method with SoundEffect.copy() and Image.copy()
  • We don't have any instance of a copy constructor

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
Copy link
Contributor Author

From @dpgeorge in #163 (comment):

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@microbit-carlos
Copy link
Contributor Author

Based on the changes from #205 (comment), we should add the copy() method to AudioRecording.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants