alexgorji / musicscore

13 stars 5 forks source link

Staff attribute not created during `part.add_chord()` #14

Closed MikeEcho closed 8 months ago

MikeEcho commented 8 months ago

I was following the tutorial examples. Example 2 claims that the exact same XML is generated as Example 1. However, I noticed a difference when doing a diff between the two XMLs, being that the staff parent is not present on the note in Example 2's XML (see below the last diff line; the other diffs are expected e.g. HW1 vs. HW2).

Screenshot 2024-01-27 at 7 14 00 pm

I looked at the expectation in https://github.com/alexgorji/musicscore/blob/master/musicscore/tests/test_part.py#L357 and just wanted to confirm that this is expected? If so, why would the staff attribute not be present on the note? If not expected, I am happy to help fix the issue.

alexgorji commented 8 months ago

@MikeEcho: Thanks for pointing this out. You are absolutely right, this is indeed an inconsistency in the output of the two examples. At this moment I'm not sure if part.add_chord() should add a staff child to the note element if only one staff is present. Staff is optional and is only necessary to be explicitly specified if there are more than one staves in one part. At this moment part.add_chord() ignores staff_number=1 if there is no staff_number=2 which I think is not a bad idea. For having an identical output of two examples it is possible to add attribute number=None to the staff object in the first one like this: Staff(number=None). I admit that it would look a little bit odd. A more complicated solution could be to refactor part.add_chord() to take its argument staff_number=1 seriously even if there are no other staves present. Then we could change part.add_chord() in the second example to part.add_chord(staff_number=1). But is it worth making these changes only for the sake of the example? I’m looking forward to knowing your opinion about it.

MikeEcho commented 8 months ago

@alexgorji In that case let's keep it as is. I'm not keen on introducing complexity when it already works, was just curious about the difference. I can offer to create a small PR to update the HW2 tutorial text to describe it :)

MikeEcho commented 8 months ago

Issue closed, as the discrepancy is expected (see above)

MikeEcho commented 8 months ago

@alexgorji I was denied permission to create and push to a remote branch in this repo - I was trying to create a PR (my dev branch into master) to update the example 2 text as discussed above. Can I be allowed, or do you prefer a different way to raise PRs?

alexgorji commented 8 months ago

@alexgorji I was denied permission to create and push to a remote branch in this repo - I was trying to create a PR (my dev branch into master) to update the example 2 text as discussed above. Can I be allowed, or do you prefer a different way to raise PRs?

Have you tried creating a fork first?

[https://opensource.com/article/19/7/create-pull-request-github]()

MikeEcho commented 8 months ago

@alexgorji Ah I shall do so. It's my first time contributing to another maintainer's repo that isn't my workplace, thanks!