Conversation
|
This looks nice, but does it really need to be in a separate module? Can we add this as a method to the |
I agree with you David that it should not. This code was initially in my modified version of the Sound class. But since I don't master the template mechanism (I think you remember some discussions around this topic some time ago ;) I couldn't find how to implement properly the extension this way. Hence this separate module version. If you give me the right way to proceed for integrating the code in the Sound class module, I can move it at the right place.
I confess not being convinced by such a merge of concerns. I prefer methods having a unique and well defined purpose which can be clearly identified by the name and signature. In our case we have on one hand a method for playing a succession of tones (for instance imitating a phone number dialing) and on the other hand a method for playing a song, based on its music score. Merging both in a single entry point which behaviour and purpose change based on the value of a parameter can be a bit confusing for the average user IMHO. |
I do remember that discussion 😺. But it so happens the
Ok, I am not insisting on this. |
|
If we want to play songs, it would be nice to add a paser that translates RTTL (https://en.wikipedia.org/wiki/Ring_Tone_Transfer_Language) to beep commands. There are lots of RTTL files online |
OK. Since the module it lives in contains markers identifying generated sections, I was thinking that it should not be edited directly. Can I modify it directly without any special consideration, apart not touching code inside the marked areas (which are not involved here as you said) ? PS: sorry for using my alternate identity for these recent comments. I'm working on professional projects at the same time from this machine, and it's a bit teddious to switch continuously between both ;-) |
|
@ensonic |
Yes, absolutely. As long as the code you are touching is outside of |
|
Fine. I'll move the code in the Sound class then and kill this branch. |
|
You can force-push new commits to this branch to update the pull request, so there is really no need to kill the branch. |
|
Hmmm, I'll need to document myself about |
|
You can basically rewrite the history of your local branch with |
|
Thanks for the help. I'll try to find my way with these indications ;-) |
|
There's no need to rebase or force-push; when a maintainer merges this PR, they can have GitHub combine all the commits into one to clean it up. |
|
|
Ahem... So which operating mode and command(s) a Git noob like me is supposed to use exactly to avoid creating a mess ? It it just :
|
|
Commit and push until you are satisfied with the results. |
|
Fine. This should be within my competences then :-) I'll manage it this evening. I guess I'll have to issue a new PR when done. Right ? |
|
Nope, when you push to the existing branch this PR will automatically update. All you need to do is continue making changes until you're happy! |
|
When will the merger be informed that I've finished with changes and that he can merge the branch into the mainstream ? Let's say that the merge is done some day, what happens if I make more pushes after that point ? Thanks all for your patience and for making me less stupid (if possible) :-) |
Anyone watching the repo is informed when you push additional commits. Until you push, they are all on your local machine. So, generally, a maintainer knows to look at it again if you push some commits that appear to attempt to respond to all feedback or you post a comment saying that you have made the changes. Generally a maintainer will submit a review indicating whether they accept the PR or request changes.
Once you have merged a PR, it "detaches" from its branch. That means that future commits to that branch do not affect the PR or other branches. If you wanted to, you could later open a new PR from that same branch to integrate changes since your last. However, that can be problematic because often the maintainer chooses to take your changes and squash them into one commit, meaning that the commits in your branch no longer are the same ones in the master branch, even though they contain the same changes. |
|
Thanks a lot WasabiFan for your time. Your explanations are crystal clear. From the last sentence, I understand that once the PR has been merged, it is safer to close the branch, and start a new one from the version of the moment if some additional work is to be done later. This would avoid the commits discrepency problem you mention. |
|
Yep, that's right. # if necessary, "git remote add upstream https://github.com/rhempel/ev3dev-lang-python
git checkout master
git pull upstream master
git checkout --branch my-new-feature |
|
Perfect. Thanks again for this "Git for the dummies" improvised course :) |
|
I've migrated the music module content in the Sound class. It works fine here on my EV3. |
ddemidov
left a comment
ddemidov
left a comment
There was a problem hiding this comment.
This looks good aside for a couple of notes I made on the changes.
ev3dev/core.py
Outdated
| #: standard US abbreviation and its octave number (e.g. ``C3``). | ||
| #: Alterations use the ``#`` and ``b`` symbols, respectively for | ||
| #: *sharp* and *flat*, between the note code and the octave number (e.g. ``D#4``, ``Gb5``). | ||
| NOTE_FREQUENCIES = _make_scales(( |
There was a problem hiding this comment.
Do this dictionary and the _make_scales() function above have use outside of the Sound class? I mean, would it make sense to move them into the class scope?
ev3dev/core.py
Outdated
| #: For instance, the note value of a eight triplet will be ``NOTE_VALUE['e'] / 3``. | ||
| #: It is simpler however to user the ``3`` modifier of notes, as supported by the | ||
| #: :py:meth:`Sound.play_song` method. | ||
| NOTE_VALUES = { |
There was a problem hiding this comment.
Should this be moved into Sound class as well?
|
Hi Denis,
You're right, it would be cleaner to put this in the Sound class, and it was what I did at first.
But I ran into problems invoking _make_scales in NOTE_FREQUENCIES declaration if _make_scales was defined as a static method of Sound. I tried various combinations without any success.
For example, a declaration such as :
class C(object):
@staticmethod
def foo():
pass
STATIC_VAR = foo()
gives the following error :
$ python test.py
Traceback (most recent call last):
File "test.py", line 1, in <module>
class C(object):
File "test.py", line 6, in C
STATIC_VAR = foo()
TypeError: 'staticmethod' object is not callable
The problem doesn't exist if foo is a module level method like hereafter:
def foo():
pass
class C(object):
STATIC_VAR = foo()
NB: this works only if foo is declared *before* Sound obviously
So we could move the tables into Sound class but the _make_scales method would have to stay at module level, and to appear *before* the Sound class declaration.
Maybe I'm missing a point and there is a right way to achieve the initial goal, but I'm not aware of it and I'd love to learn it.
NB: NOTE_VALUES is not involved in these problems. I treated it the same way as its companion variable by homogeneity.
Apart of that, I've read quite a large number of threads explaining that :
- trying to encapsulate everything in classes for namespacing's sake is a habit of people coming from languages such as Java,
- it is not justified in Python
- moreover it is less efficient since adding one more level of indirection when accessing the involved data.
Advocates of this position advise to use well chosen identifiers instead if stressing the logical link with the class is important. Even if I wrote a quite huge amount of Java code in the past, I would tend to be convinced by their argumentation.
I confess that I made something wrong however : I should have prefixed these tables with '_', since they are for internal use only. Naming things like gives the right to put stuff at module level even if tied to classes, since the '_' tells to the reader : "this declaration is not code you are looking for", in other words, "don't pay attention to this and skip over it". Generally, I put such declarations at the end of the modules is possible.
Anyway, if you think the style would be more in line with the project practices, I have no problem with refactoring this part of the module and embedding the tables in the Sound class. Just let me know.
Best regards
Eric
…----- Mail original -----
De: "Denis Demidov" <[email protected]>
À: "rhempel/ev3dev-lang-python" <[email protected]>
Cc: "Eric PASCUAL" <[email protected]>, "Author" <[email protected]>
Envoyé: Mercredi 15 Février 2017 07:05:51
Objet: Re: [rhempel/ev3dev-lang-python] music module added (#300)
@ddemidov commented on this pull request.
This looks good aside for a couple of notes I made on the changes.
In ev3dev/core.py : > +def _make_scales(notes):
+ """ Utility function used for building the note frequencies table """
+ res = dict()
+ for note, freq in notes:
+ freq = round(freq)
+ for n in note.split('/'):
+ res[n] = freq
+ return res
+
+#: Note frequencies.
+#:
+#: This dictionary gives the rounded frequency of a note specified by its
+#: standard US abbreviation and its octave number (e.g. ``C3``).
+#: Alterations use the ``#`` and ``b`` symbols, respectively for
+#: *sharp* and *flat*, between the note code and the octave number (e.g. ``D#4``, ``Gb5``).
+NOTE_FREQUENCIES = _make_scales((
Do this dictionary and the _make_scales() function above have use outside of the Sound class? I mean, would it make sense to move them into the class scope?
In ev3dev/core.py : > +#: to obtain subdivisions, given the corresponding symbolic identifier:
+#:
+#: = ===============================
+#: w whole note (UK: semibreve)
+#: h half note (UK: minim)
+#: q quarter note (UK: crotchet)
+#: e eight note (UK: quaver)
+#: s sixteenth note (UK: semiquaver)
+#: = ===============================
+#:
+#:
+#: Triplets can be obtained by dividing the corresponding reference by 3.
+#: For instance, the note value of a eight triplet will be ``NOTE_VALUE['e'] / 3``.
+#: It is simpler however to user the ``3`` modifier of notes, as supported by the
+#: :py:meth:`Sound.play_song` method.
+NOTE_VALUES = {
Should this be moved into Sound class as well?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub , or mute the thread .
|
|
This works: class C(object):
@staticmethod
def foo():
pass
STATIC_VAR = C.foo() |
|
Are you sure ?
I get this instead :
Traceback (most recent call last):
File "test.py", line 4, in <module>
class C(object):
File "test.py", line 9, in C
STATIC_VAR = C.foo()
NameError: name 'C' is not defined
I tried both on Python 3 and Python 3.
It seems logical since at the moment STATIC_VAR definition is processed, the C class is not yet defined, since in the process of being.
…----- Mail original -----
De: "Denis Demidov" <[email protected]>
À: "rhempel/ev3dev-lang-python" <[email protected]>
Cc: "Eric PASCUAL" <[email protected]>, "Author" <[email protected]>
Envoyé: Mercredi 15 Février 2017 10:57:34
Objet: Re: [rhempel/ev3dev-lang-python] music module added (#300)
This works:
class C ( object ): @ staticmethod def foo (): pass STATIC_VAR = C.foo()
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub , or mute the thread .
|
|
Ah, my bad. I was experimenting in an interactive shell, and the |
|
So it seems we do have to keep |
|
I regularly fall in this trap too :)
…----- Mail original -----
De: "Denis Demidov" <[email protected]>
À: "rhempel/ev3dev-lang-python" <[email protected]>
Cc: "Eric PASCUAL" <[email protected]>, "Author" <[email protected]>
Envoyé: Mercredi 15 Février 2017 11:06:09
Objet: Re: [rhempel/ev3dev-lang-python] music module added (#300)
Ah, my bad. I was experimenting in an interactive shell, and the C I got was a leftover from previous attempt. So, I was wrong, and it does not work.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub , or mute the thread .
|
|
No problem for refactoring the code the way you prefer. I'll take care of this ASAP. |
|
Done. Static tables moved into sound class. |
|
Thanks! |
5 participants
Hello,
This module adds a function which makes easier to play songs by writing them using the conventional musical notation (note and duration). A basic demo illustrating it (and a couple of the other sound related ev3dev features) is added in the demo tree.