openlilylib / lilypond-export

LilyPond export API to write Humdrum, MusicXML and more
GNU General Public License v3.0
22 stars 8 forks source link

Refactor and use SXML more thoroughly #18

Closed PaulMorris closed 5 years ago

PaulMorris commented 5 years ago

Refactor the MusicXML.scm module to:

These changes make the code more functional/scheme-ish, and (hopefully) easier to build on and maintain. There's a lot here, but looking at the changes commit-by-commit will help.

I used xmldiff on the command line to confirm that the XML output from the export-example.ly was the same before and after these changes. (This xmldiff tool will be helpful for this project in general.)

There's room for improvement, but this finishes the conversion to SXML. (I tried converting the top two ?xml and !DOCTYPE lines to SXML but there seems to be no obvious way to do the DOCTYPE one, despite searching the web and looking through the Guile manual.)

rshura commented 5 years ago

This looks really good to me, thanks!

I made one more commit with whitespace/alignment fixes. It's super-minor, please feel free to revert if you don't like this.

PaulMorris commented 5 years ago

Thanks for the review! On the whitespace/alignment, I just use the auto-formatting feature in Frescobaldi. I'm pretty sure it uses the standard Scheme formatting style. Sometimes the results are mysterious to me, but it's so nice to have a tool to do it automatically. So my preference would be to stick with the auto-formatting results.

rshura commented 5 years ago

Interesting. I use emacs, and that's what the scheme mode in emacs wanted to do. I'll revert my commit in a bit.

PaulMorris commented 5 years ago

Ah, well, in that case, I'm sure emacs is better equipped for scheme than frescobaldi, so we should probably follow it. Also, I've been using spacemacs for several months now, and it would help motivate me to learn how to really use it well for programming in scheme. So, I'd say no need to revert your commit.

PaulMorris commented 5 years ago

Aha, there's an issue to improve the Scheme indentation in Frescobaldi: https://github.com/wbsoft/frescobaldi/issues/764

rshura commented 5 years ago

OK, in that case I'll leave it in. It's really very minimal changes in whitespace :-)

On the subject of XML differences: as I was debugging my next tweaks, I tried xmldiff and it had my machine completely unresponsive, until it was OOMed by the kernel. Seems like it's only good if there's no differences or very few. The man page says don't use for anything with more than 100 nodes :-)

The xmllint though is a nice tool: xmllint --format produces a nice readable output, and then the regular diff can examine those. It's not like we have room here for equivalent-but-not identical changes.

PaulMorris commented 5 years ago

Ah, good call on using xmllint @rshura . I've added another commit to this PR ( 73b6a17 ) that removes the writeln function and just uses display instead, now that we only use it in two places.

PaulMorris commented 5 years ago

I got the thumbs-up from @jpvoigt on email, so I've now merged this PR.