-
Notifications
You must be signed in to change notification settings - Fork 61
Update Regexp for Ending #588
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
Conversation
This updates the regular expression for the `<ending number="...">` attribute, to allow for multiple spaces after the comma: `<ending number="2, 3">` The schema change is finished -- I want to integrate the doctools better into a testing eco-system.
|
This is wrong. Tokens "have no internal sequences of two or more spaces." See the definition at https://www.w3.org/TR/xmlschema-2/#token. |
mdgood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please close or delete this PR as it violates the definition of an XML Schema token.
|
Hi Michael -- I'll do that, but can you explain why the first half of the regex makes sense then: As I now understand it, if If these regex's are going on post normalized, should it not be |
|
That case is to cover the empty string. I don't remember why it was specified that way instead of what you propose - if that wasn't a valid schema regex, if there was a problem with parser software handling it, or I just used the first thing I thought of. It may not be ideal, but at least let's not make it any worse. 😀 |
Agreed -- at least you can see that I wanted to make both sides consistent. We'll go the opposite direction. But this ends up not being a change requiring a XST since the parser will trim it in 4.0 and in 4.1 |
|
The docs changes got caught up in the move from Windows line-endings to Unix. Here's the relevant added lines in the docs: |
| </xs:annotation> | ||
| <xs:restriction base="xs:token"> | ||
| <xs:pattern value="([ ]*)|([1-9][0-9]*(, ?[1-9][0-9]*)*)"/> | ||
| <xs:pattern value="()|([1-9][0-9]*(, ?[1-9][0-9]*)*)"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it makes sense to change ' ?' to '[ ]?' to increase readability.
| <li>The <a href="../../musicxmlreference/data-types/ending-number/">ending-number</a> type used | ||
| in the <code>number</code> attribute of the | ||
| <a href="../../musicxml-reference/elements/ending/"><ending></a>, previously had a regex | ||
| that implied that empty spaces were different from the empty string. This has been clarified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Empty spaces' sounds like a pleonasm 🙂
I suggest to replace this with 'a sequence of consecutive space characters' or something like that.
|
I still don't think we should make this change as I don't see it adding value, only risk. It doesn't increase the power of what MusicXML can represent and this hasn't been a significant point of confusion in 17 years. Any change requires testing to make sure it works and documentation to explain the change. The cost isn't worth it. |
|
Agreeing with Michael -- not sufficient improvement to make a change that could break some (non-conforming) code out there. Closing. |
This updates the regular expression for the
<ending number="...">attribute, to allow for multiple spaces after the comma:<ending number="2, 3">The schema change is finished -- I want to integrate the doctools better into a testing eco-system.
See #580