Skip to content

Simplify and speed up hot functions GenerateRandomID() and CreateXmlTag()#286

Merged
ahankinson merged 1 commit intomusic-encoding:developfrom
notengrafik:pr/streamline-xml-generation
Feb 9, 2026
Merged

Simplify and speed up hot functions GenerateRandomID() and CreateXmlTag()#286
ahankinson merged 1 commit intomusic-encoding:developfrom
notengrafik:pr/streamline-xml-generation

Conversation

@th-we
Copy link
Member

@th-we th-we commented Feb 7, 2026

This changes attributes quotes from double to single quotes, but avoids a lot of string concatenation and Chr() function calls.

…ag()

This commit uses single quotes for attributes instead of double quotes
@th-we th-we requested a review from rettinghaus February 7, 2026 07:01
@@ -405,10 +405,8 @@ function EncodeEntities (string) {


function GenerateRandomID () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not Random, so maybe better call it GenerateID.

@ahankinson
Copy link
Member

What’s the motivation for changing to apostrophes?

@ahankinson
Copy link
Member

If you want to avoid all the calls to Chr(34), just assign it to a variable? I find XML that uses single quotes to be really ugly.

@th-we
Copy link
Member Author

th-we commented Feb 8, 2026

I have no aesthetic attachment to either single or double quotes. In fact, it's possible to also escape double quotes, which I did in #289. (By the way, I opened that new pull request because I don't like pushing a commit to this branch that immediately reverts the previous commit, so there's some aesthetic preferences of mine ;-).)

@ahankinson
Copy link
Member

If it makes no difference, then double quotes would be preferable as they are more in line with common practice (and also aligns with Verovio).

@th-we
Copy link
Member Author

th-we commented Feb 9, 2026

If we're all happy with the double quote pull request, I'd close this one and merge the other one.

@rettinghaus
Copy link
Member

I'm ok with that.

@ahankinson ahankinson merged commit d12d256 into music-encoding:develop Feb 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants