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

Clean up some "OLL-package" related details #17

Merged
merged 3 commits into from
Oct 29, 2018
Merged

Conversation

uliska
Copy link
Contributor

@uliska uliska commented Oct 29, 2018

Sorry for making this a PR to add to my "Hackaton" balacne ;-)

Objectives:
- limit line lengths
- improve readability
(ly:run-translator (ly:score-music (scorify-music music)) (FileExport `((filebase . ,filebase)(exporter . ,exporter)) ))
))
#(let
((exporters
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpvoigt I have just started looking into the code, but I have a suggestion here. I think the current approach to provide and register the export functions is tedious, error-prone and not very "plugin-like". I'd replace this explicit alist with something like ((exporters (export-functions))) with

  • export-functions being defined in api.scm
  • moving the export modules (Humdrum.scm etc.) to a subdirectory (changing the module path from lilypond-export Humdrum to, say, lilypond-export exporters Humdrum)
  • having api.scm load all modules it can find in the subdirectory
  • require an export module to export a function with a generic name (export instead of exportMusicXML)
  • have export-functions create the alist from the export functions in all found export modules.

This way adding a new export target would not require adding information in three different places but only to create a new module with that one required function and drop that into the exporters directory.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this is a good idea. This part is still from the code for my presentation at the MEI conference in Tours last year. We should go this way, but talk with Alex and Paul before that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should do it like that, but talk with Alex and Paul before that.
Thanks for the cleanup!

Copy link
Contributor Author

@uliska uliska Oct 29, 2018

Choose a reason for hiding this comment

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

After starting to read into the MusicXML module and seeing how massive the export routines will become when approaching anything like comprehensive coverage I suggest yet another approach.

The current code (and my previous suggestion) first loads all registered export modules and then select the appropriate function. Instead the process should be the following:

  • check if exporter is a symbol or a procedure
  • if it is a procedure simply use that (maybe make that more explicit)
  • if it is a symbol then to something like set!/let exporter (export-function exporter)
  • where export-function (defined in api.scm) loads the corresponding module and returns the generically named function. The "exporter" key then has to match the module name exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PaulMorris @rshura what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create a PR then it'll be easier to assess what I think of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I just don't understand it: The MWE works perfectly, but if I transfer it to the package files it stops doing so ...

If I have two files:

; my-mod.scm

; This is visible outside
(define-public (my-public-procedure)
  (display "my-mod my-procedure\n"))

; This not
(define (my-local-procecure) (display "local"))

and

% test.ly
#(load-from-path "/absolute/path/to/my-mod.scm")
#(display my-public-procedure)

#(my-public-procedure)

#(my-local-procedure)

the procedure with define-public is visible from the LilyPond file and the other isn't (as it should be). But in the code applied to the actual package it doesn't work, and the function from the loaded module is not visible. I'm right now creating a pull request that demonstrates this problem. Maybe someone has a solution...

This relies on oll-core being updated, currently to the state
of openlilylib/oll-core#41
@jpvoigt jpvoigt merged commit b69b579 into master Oct 29, 2018
uliska added a commit that referenced this pull request Oct 29, 2018
This doesn't work yet, but I want to submit it for review.
What I think is the right thing is how package.ily goes along with
choosing the "backend"/target.

I also think that FileExport in api.scm does the right thing.

What does *not* work is making the export-lilypond function from
within Humdrum.scm (so far this only touches the Humdrum file)
available to api.scm. I really don't understand it because in my MWE
it *did* work. (see #17 (comment))
@rshura
Copy link

rshura commented Oct 30, 2018

Already commented in #19 but copying here too just in case.

I have to admit that I got no clue about modules, packages, and the best way to call things, either in scheme in general or in LP in particular. I'm very new to all of this, and I'm sure you guys will know how to expose this the best. Sorry, but I just don't have enough expertise and intuition to offer here :-)

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