Skip to content
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

adapt instrDef to staffDef #933

Merged
merged 1 commit into from
May 24, 2023

Conversation

rettinghaus
Copy link
Member

@rettinghaus rettinghaus commented Mar 17, 2022

Using att.nNumberLike instead of att.nInteger for instrDef seemed a bit inconsistent, as staffDef and layerDef are part of the latter.

This PR proposes to model it like staffDef instead of using att.common.

@github-actions github-actions bot added the Component: Core Schema changes to source/modules/* (assigned automatically) label Mar 17, 2022
@rettinghaus rettinghaus added this to 2022-03-25 ODD Friday in ODD Meetings Mar 21, 2022
@bwbohl bwbohl added the Backwards-Compatibility: breaking discussed or proposed changes would result in a backward incompatibility / require a major release label Apr 22, 2022
@bwbohl
Copy link
Member

bwbohl commented Apr 22, 2022

Like the harmonisation, nevertheless dislike that you had to reference all those other classes just to change one of them…

Seems to me that maybe we should rethink the attribute classes at some point, especially the need of two versions of @n.

@rettinghaus
Copy link
Member Author

True. There are several elements that are actually affected here (e.g., keyAccid). Maybe it would be better to have att.nInteger in att.common and switch on att.nNumberLike in those specific cases we actually want to allow this?

@rettinghaus rettinghaus marked this pull request as draft April 22, 2022 09:40
@kepper
Copy link
Member

kepper commented Apr 22, 2022

But that is a separate discussion, isn't it? What I would propose is to have two flavors of att.common, which can be referenced as needed. Regular att.common would bring att.nNumberLike (so we have very few changes throughout the specs), and prospective att.common.nInteger (or some such) would bring att.nInteger.

@ahankinson
Copy link
Member

@rettinghaus could you remove the draft label?

@rettinghaus rettinghaus marked this pull request as ready for review May 24, 2023 18:38
@rettinghaus
Copy link
Member Author

@ahankinson with pleasure!

@ahankinson ahankinson merged commit c1b6edd into music-encoding:develop May 24, 2023
@ahankinson
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards-Compatibility: breaking discussed or proposed changes would result in a backward incompatibility / require a major release Component: Core Schema changes to source/modules/* (assigned automatically)
Projects
No open projects
ODD Meetings
  
2022-03-25 ODD Friday
Development

Successfully merging this pull request may close these issues.

None yet

5 participants