Closed fedelibre closed 9 years ago
Thank you for the contribution. There are a few issues to discuss but not actually with your commit:
Tablature
really the best name?\version
string is newer than the LilyPond version used for testing. This is not your fault but means that the green "All checks have passed" doesn't mean anything.#(ly:message)
is really ugly.Apart from that everything looks OK. So I'll wait for your comment on the title and try to update the automated tests in the meantime.
OK, I've updated the automated test to use 2.19.30, and (not surprisingly) the test still passes. So it's just the question of the library name before actually merging the PR.
The other two issues (documenting and that bug) will still affect that library but are not related to merging it.
Ah, one more issue: It seems your function works with 2.18.2 as well. Please check if that's the case, and if so, please update the version strings accordingly. You might even check if it runs on 2.16 if you want.
And one more:
In __main__.ily
you include the quarter-tone file, so when loading the library this is already available. In the usage-example you also use \useModule
afterwards which is unnecessary. So you'd have to decide what you considere the proper interface:
\useLibrary Tablature
already includes the quarter-tone snippet (and any other snippets that might be added)\useModule tablature.quarter-tones
is necessary.I'll update the \version statements, as it works with 2.18.2 as well. This makes this snippet useful, because there's a patch in review to add quarter tones in tablature in 2.19.x.
I would wait for this patch to be included in lilypond before updating this PR, so I can write on the README that this snippet is needed for versions below 2.19.
In this case, Tablature is a good name choice, as this snippet adds to TabStaff a feature (quarter tones) already available in Staff. But bending is not so easy to be categorized. I considered two options:
and I chose the first. Other ideas for the library name:
It depends on the scope of the library. If the library must be as "inclusive" as possible, I would use Tablature. If it must be as specific as possible, I would use Microtonality for quarter tones (renaming the file quarter-tones-tablature...) and Effects for bending.
Urs, just in case it was not clear: I'm waiting for your comments on the library name before updating the PR. Should it be inclusive or specific? I don't have strong opinions on either of these and don't know what kind of issues the old oll categorization had.
Harm's patch is on countdown and should enter version 2.19.31
Oh, I rather had the impression you were waiting for Harm's patch.
Regarding the naming I would say that both tablature
and strings
would be good names, while the other two suggestions would probably be misleading.
And a decision between strings and tablature would not primarily be about what is more appropriate to your initial function but what you'd expect to be added in the future. Will this become a collection of tablature tools or string notation tools?
The problem with the directory naming in openLilyLib so far is that it is so random. If I have a new function and don't immediately see where to add it I can simply create a new directory. This has led to the situation that it's often barely possible to guess where a function is that I recall being somewhere in OLL.
The new library approach should really be more clear and strict about naming and "access paths". A library is supposed to be an "entity" with
https://gridly.openlilylib.org
Ok, I'll keep tablature
.
I've squashed the commits into a single one and pushed the changes. I've renamed "quarter tones" to "microtones", as it's more correct and a better keyword, I think.
I think that name is actually best.
Two things will have to be considered (one of which requires your decision).
Harm's patch has been pushed and it's in version 2.19.31. I explained it in the README. What is a version switch? Ideally, microtones.ily should check the version in the input file and print a message if it's >=2.19.31. Is it doable in Scheme?
Otherwise I may just print a message for any compilation:
#(display "The tablature.microtones module should be used only with LilyPond versions earlier than 2.19.31. Any later version has built-in support for microtones in tablature.")
A version switch is something that openLilyLib provides. It's a set of Scheme predicates that allows to execute code based on the actually executed LilyPond version.
I'll add something and then merge the branch so you can have a look how it is done. I will try to figure out the best behaviour but maybe you could tell me directly. Should lilypond-greater-than? 2.19.30
?
First option is the best: if greater than 2.19.30, it should print a deprecation-like message but still compile the file.
The third option won't work, because the built-in function requires a specific setting - noteToFretFunction - in the layout block.
OK, I've adde something and merged the branch. Thank you for the contribution.
BTW: @fedelibre You know that you are already a team member, so you wouldn't have to work on a fork (this would actually make things even easier for review)?
I know that I'm a team member but what's the workflow to get a review in this case? I should push to a branch in the openlilylib repository? And then?
You will find a notice on the repository's front page about your new branch. This allows you to open a pull request.
Otherwise you can simply go to "Pull Requests" and click on New Pull Request. Then you can choose two branches to compare.
@fedelibre One more question I forgot to ask before: I see you have an empty __main__.ily
file - which indicates that loading the library doesn't actually load any functionality and the user has to explicitly load any modules. This is of course a valid use case, but: did you find it annoying? Should we provide a way that the "main" file can simply be left out if it doesn't do anything?
I mean, in Python you regularly have empty __init__.py
files to properly announce modules, but we also have the __init__.ily
file which originally was optional but now holds the library/module declaration and is therefore mandatory now.
Actually I had already considered going in that direction but was too lazy so far ...
Well, an empty file doesn't have much sense. Yes, I think that it would be desirable.
@uliska The check you added prevents the compilation on 2.19.31:
Processing `/home/fede/src/openlilylib/snippets/ly/tablature/usage-examples/microtones.ly'
Parsing...
openLilyLib: library infrastructure successfully loaded.
loaded
/home/fede/src/openlilylib/snippets/ly/tablature/microtones.ily:414:1: warning: openLilyLib:
The module
tablature.microtones
has been integrated to LilyPond 2.19.31. You can still use the openLilyLib version but if you
constantly use newer LilyPond versions you should consider using the built-in function now.
\microtonesCheckLilyVersion
Interpreting music...[8][16]
Preprocessing graphical objects.../home/fede/src/openlilylib/snippets/ly/tablature/microtones.ily:374:25: In procedure string->number in expression (string->number nmbr):
/home/fede/src/openlilylib/snippets/ly/tablature/microtones.ily:374:25: Wrong type argument in position 1 (expecting string): ((#<procedure vcenter-markup (layout props arg)> "") (#<procedure vcenter-markup (layout props arg)> (#<procedure fontsize-markup (layout props increment arg)> -2.5 "1/2")))
This is interesting. The code did work with my self-compiled version standing somewhere between 2.19.30 and 2.19.31. Now I pulled to the latest version and got the same result as you.
Would you be able to track down what changed in this code recently?
I added some more commits that suppress the crash, but I'm quite sure this isn't a proper cure.
@davidnalesnik would you mind having a look at d5f32aa2 ?
The first module added is "quarter tones".