\compressFullBarRests should be renamed (issue 553750044 by v.villenave@gmail.com)

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

\compressFullBarRests should be renamed (issue 553750044 by v.villenave@gmail.com)

Dev mailing list
LGTM, thanks!  I have some nits here and there, though.


https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely
File Documentation/changes.tely (right):

https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely#newcode66
Documentation/changes.tely:66: @code{\compressFullBarRests} has been
renamed
s/renamed/renamed to/

https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely#newcode67
Documentation/changes.tely:67: @code{\compressEmptyMeasures}, to clear
up
s/, to clear up/to avoid/

https://codereview.appspot.com/553750044/diff/561590043/Documentation/changes.tely#newcode70
Documentation/changes.tely:70:
Shouldn't \expandEmptyMeasures be mentioned as well?

https://codereview.appspot.com/553750044/diff/561590043/Documentation/de/notation/rhythms.itely
File Documentation/de/notation/rhythms.itely (right):

https://codereview.appspot.com/553750044/diff/561590043/Documentation/de/notation/rhythms.itely#newcode891
Documentation/de/notation/rhythms.itely:891: @funindex
expandEmptyMeasures
I suggest to fix the wrong index entries here: Only retain the ones with
a leading backslash.

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/rhythms.itely
File Documentation/notation/rhythms.itely (right):

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/rhythms.itely#newcode868
Documentation/notation/rhythms.itely:868: the note name uppercase
@code{R}.  Their duration is entered
For single-letter stuff I suggest to use `@example` instead of `@code`.

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/rhythms.itely#newcode888
Documentation/notation/rhythms.itely:888: number of measure-lengths;
therefore, some time signatures
I guess you refer to the `measure-length` property, right?  If this is
correct, then you should write `@code{measure-length}` instead.

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely
File Documentation/notation/staff.itely (right):

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode864
Documentation/notation/staff.itely:864: Methods to quote other voices
and format cue notes
Maybe s/format/to format/.

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1586
Documentation/notation/staff.itely:1586: @code{\compressMMRests} will
only apply to rests, and leave
no comma

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1588
Documentation/notation/staff.itely:1588: a property setting, its syntax
differs slightly in that
no comma

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1603
Documentation/notation/staff.itely:1603: rely on the
@code{@var{skipBars}} internal property, which is
why @var?

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1605
Documentation/notation/staff.itely:1605: @ref{The set command}.
ditto

https://codereview.appspot.com/553750044/

Reply | Threaded
Open this post in threaded view
|

Re: \compressFullBarRests should be renamed (issue 553750044 by v.villenave@gmail.com)

Dev mailing list
Thanks for picking this up and taking it over the finishing line,
Valentin. LGTM apart from a couple of nits.

Trevor


https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely
File Documentation/notation/staff.itely (right):

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1559
Documentation/notation/staff.itely:1559: to be explicitely printed,
using the syntax described in
explicitly

https://codereview.appspot.com/553750044/diff/561590043/Documentation/notation/staff.itely#newcode1589
Documentation/notation/staff.itely:1589: it requires to be followed by a
music expression:
requires to -> must

https://codereview.appspot.com/553750044/

Reply | Threaded
Open this post in threaded view
|

Re: \compressFullBarRests should be renamed (issue 553750044 by v.villenave@gmail.com)

v.villenave
In reply to this post by Dev mailing list
Reviewers: lemzwerg, Trevor Daniels,

Message:
Thanks guys, it means a lot! (Particularly from Trevor, who opened the
tracker page in the first place.)

Since I had both your LGTMs, I’ve pushed my patch onto staging as
http://git.savannah.gnu.org/cgit/lilypond.git/commit/?h=staging&id=83045b846acb4aaadf54373941a915c4c45fb522
(then I realized the review window was technically still open until
tomorrow. Sorry about that.)

I’ve taken into account all your suggestions, with only a couple of
additional comments:

On 2020/03/21 18:27:07, lemzwerg wrote:
> I guess you refer to the `measure-length` property, right?  If this is
correct,
> then you should write `@code{measure-length}` instead.

Hm.  Measure-length was the wording used so far, but it’s really more of
an internal property than something we want to emphasize as
user-exposed; it’s not documented anywhere in the NR other than through
grob-properties.scm (not even a regtest).  So I’d rather rewrite the
sentence as "the length of a measure", which is wordier but a bit
clearer I think.

> why @var?

’coz I keep misreading the CG :-/

On 2020/03/22 11:07:23, Trevor Daniels wrote:
> Thanks for picking this up and taking it over the finishing line,
Valentin.

My pleasure.  Hopefully I’ll be able to contribute a bit more in the
next few weeks…
I hope you’re well; take good care of yourself… and stay away from
London!

Cheers,
V.

Description:
\compressFullBarRests should be renamed

- Rename \compressFullBarRests to \compressEmptyMeasures
as suggested by Trevor in #4375.

- Document the new command (and explain its difference
with \compressMMRests) by creating a new subsubsec in
NR 1.6.3 "Writing parts".

- Add index entries and links everywhere I can think of,
obviously starting with NR 1.2.2.3 "Full measure rests".

- Add convert rule and update syntax through the doc.

- Clarify the (in)famous progerror
"Multi_measure_rest::get_rods (): I am not spanned!"
since a) the function it refers to has changed anyway
b) its wording’s never been particularly helpful IMO.

- This patch will require po-update and makelsr at
some point (and snippets/new should be checked for
duplicate stuff now that the LSR’s been updated).

Please review this at https://codereview.appspot.com/553750044/

Affected files (+196, -98 lines):
  M Documentation/changes.tely
  M Documentation/de/changes.tely
  M Documentation/de/notation/changing-defaults.itely
  M Documentation/de/notation/rhythms.itely
  M Documentation/de/notation/staff.itely
  M Documentation/fr/changes.tely
  M Documentation/it/changes.tely
  M Documentation/ja/changes.tely
  M Documentation/ja/notation/rhythms.itely
  M Documentation/notation/ancient.itely
  M Documentation/notation/changing-defaults.itely
  M Documentation/notation/rhythms.itely
  M Documentation/notation/staff.itely
  M Documentation/snippets/multi-measure-rest-length-control.ly
  M Documentation/snippets/new/multi-measure-rest-length-control.ly
  A + Documentation/snippets/new/numbering-single-measure-rests.ly
  M Documentation/snippets/numbering-single-measure-rests.ly
  M input/regression/merge-rests-engraver.ly
  M input/regression/merge-rests-magnify-staff.ly
  M input/regression/multi-measure-rest-number-threshold.ly
  M input/regression/rest-hanging-breve.ly
  M lily/multi-measure-rest.cc
  M ly/property-init.ly
  M python/convertrules.py