#4603 change all occurences of ‘partcombine’ to ‘partCombine’.

On 13/08/15 21:42, Malte Meyn wrote:

Hi list,

the case of the ‘c’ in partcombine is inconsistent, confusing me every
time I use \partcombine(Apart|Chords|…):

\partcombine, \partcombineApart, … but
\partCombineTextsOnNote, \partCombineListener

I would suggest to change all occurences of ‘partcombine’ to ‘partCombine’.

Why not change ‘partCombine’ to ‘partcombine’? Because I looked at the
engravers which use the more ‘natural’ underscore instead of camelCase
for spaces. And in the same way as ‘Cue_clef_engraver’ suggests that
‘cue clef’ are two words (resulting in ‘cueClefGlyph’, not
‘cueclefGlyph’), ‘Part_combine_engraver’ does.

I think that this would be rather easy to change.

Cheers,
Malte

New Rietveld - http://codereview.appspot.com/369930043

[Trevor Daniels]

The comparison of the names with the hyphenated and underscored names used in part-combine-iterator.cc and part-combine-engraver.cc, like

:::TeX
class Part_combine_iterator : public Music_iterator

does indeed support renaming partcombineXx to partCombineXx. Seems like a sensible low-priority change which could be easily accommodated with a convert-ly rule. Any comments from users of partcombine?

[Charles Winston]

I've uploaded a patch aiming to fix this. Changed all instances in the source code and wrote a convert-ly rule to go along with it.

[Charles Winston]

Response to Issue #4603: Syntax change from all instances "partcombine" to "partCombine" and convert-ly rule added to go along with.

http://codereview.appspot.com/323040043

[Anonymous]

This fails the make stage.

Charles there needs to be changes done within the documentation - as Trevor pointed out in the Rietveld issue - but not just the 'descriptive' part (i.e. for the readers) but also to help build the documentation itself (and the website too). As the documentation is built using LillyPond (via LilyPond-book) to create all the musical examples/images in much of the doc, there are parts that will be still using the 'old' \partcombine command.

Also we have to consider 'snippets' too - many also use the 'old' command.

Let me know if you need help with this, I can maybe spare some time this weekend.

[Anonymous]

Change all instances of \partcombine to \partCombine in the documentation

Issue 4603 (2 of 3)

All instances of \partcombine in
../Documentation (including translated
and snippets) have been changed to
\partCombine.

http://codereview.appspot.com/326870043

[Anonymous]

Change all instances of \partcombine to \partCombine in the reg tests

Issue 4603 (3 of 3)

All instances of \partcombine in
../input/regression/ have been
changed to \partCombine.

http://codereview.appspot.com/327970043

[David Kastrup]

Ok, this is really really late but I've taken a look at the output of

git grep '[pP]artCombine'

in the current code base and changing those occurences to partcombine would be way less effort than the other way round: basically most of them are new.

I might be predisposed by lots of exposure to previous code though. The oldest ones to be changed would be partcombineListener (output definitions are not capitalized?), PartcombineMusic, PartcombinePartMusic, printPartcombineTexts.

I am slightly leaning to that change but it would also have to result in Partcombine_engraver, Partcombine_iterator, Partcombine_part_iterator to be consistent. Basically making partcombining one word (wouldn't separate words be rather "combine parts"? The correct spelling of partcombiner would likely be "the part-combiner". Huh).

However that may be: having a large discrepancy without backward compatibility between 2.18 and 2.20 seems like a bad idea since LSR and online help will bomb a lot. We have seen this effect with various backward-compatible but compelling changes in the past: having this in a non-backward-compatible way seems like a bad idea that will cause a whole lot of medium-term pain for little long-term gain.

So I think it would be reasonable to
a) postpone any such changes to 2.21. Since we currently are at a good time to fork off the 2.20 release branch anyway, that does not need to imply a large delay.
b) develop a mechanism for backward compatibility. Reasonably easy for commands, trickier for properties and engravers.

Point b) should be tackled rather fast though, preferably within a reasonable time frame of a), perhaps even before if possible. Depends on how fast we can come up with ideas for making this work.

[Anonymous]

I'll set this to 'Waiting' for now as I have no problems with waiting until 2.20 is released and there is no pressing requirement for what has been done so far.

[Anonymous]

David,

This had been sat at Waiting for a whiile, I kind of stopped 'noticing' why. Now I see we have 2.21 are you saying - in effect - that this patch as it stands (assuming it still applies) is too little or could we apply this and then keep the tracker open, or create a new tracker for the 'rest' of your observation/suggestions?

It seems a waste to let this patch go by-the-by if it is usable.

[David Kastrup]

Still cherry-picking stuff into the 2.20 branch, so we don't want extensive departures (in particular large convert-ly orgies) in master yet. Once 2.20.1 is out, this can get back on the slate, but I'd actually want to get 2.21.1 (pretty much right after 2.20.1, however) without such changes in it as well.

[Valentin Villenave]

Change \partcombine (et al.) to \partCombine

I figured I’d try and rebase/update/complete Charles’ previous
proposal in case someone ever decides to apply it.
In which case, I guess \autochange should be updated as well;
that’s the only other multi-word command I could find that
hadn’t been previously camelCased as part of Graham’s
`Grand Syntax Stabilization’ project a few years back.

… Well, unless you count \defaultchild and \grobdescriptions,
but that’s not something the average user tends to be exposed to.

Then again, for some reason we do still have \fontsize and
\normalsize, all lower-case (camelCase being precisely what
sets apart fontSize, as a property, from \fontsize as a command).

I personally don’t have more than a slight preference towards
\partCombine and \autoChange, and I wouldn’t dare to guess
how much damage this syntax break could cause (though my guess
would be that we’ve definitely seen worse in the past, this one
being fairly trivially handled by convert-ly).

And, yes, I’m well aware that there still are much bigger
fishes to fry at the moment (but that’s already what
got the \partcombine patch stuck in limbo last year).

http://codereview.appspot.com/369930043

[Anonymous]

This fails make doc.

partcombine-and-autobeamoff.ly

has a '\partcombine' command (instead of '\partCombine') which breaks make doc.

I did a makelsr after applying and before 'make test' but I didn't run a separate convert-ly (that is not so easy to do in the middle of a patch test) so I assumed that convert-ly run would be included in the patch?

[Valentin Villenave]

facepalm -- "needs_work", indeed.

Here’s a revamped patch that passes make & makedoc, and adds \autoChange.

http://codereview.appspot.com/369930043

P.S. Hi James!

[Anonymous]

Passes make, make check and a full make doc.

[Anonymous]

Patch on countdown for Dec 21st