Hi Carl,
Carl Sorensen wrote:
On 12/1/09 5:07 PM, "Ian Hulin" <ian@xxxxxxxxxxxx> wrote:
Hi Carl,
Your last message came through in stereo stereo. . .
Carl Sorensen wrote:
Thanks, Ian.
The patch looks good to me as far as it goes.
On 11/30/09 4:31 PM, "Valentin Villenave" <v.villenave@xxxxxxxxx>
<mailto:v.villenave@xxxxxxxxx> wrote:
On Mon, Nov 30, 2009 at 11:04 PM, Ian Hulin <ian@xxxxxxxxxxxx>
<mailto:ian@xxxxxxxxxxxx> wrote:
Patch for review on Rietveld??? http://codereview.appspot.com/164045
Don't forget to document the new shortcuts, and to add an item to
changes.tely :)
Strictly speaking, there's not a requirement for developers to write their
own documentation (although we appreciate it).
However, there *is* a requirement for developers to write their own
regtests. I didn't see any regtests (either new or modified) in the patch.
That's because there don't seem to be any specifically for these functions.??
There are two regression tests in which RemoveEmptyStaffContext is used:
??hara-kiri-percent-repeat.ly and hara-kiri-pianostaff.ly.
Nothing tests RemoveRhythmicStaffContext
I can add?? DrumStaff, TabStaff and RhythmicStaff?? and the corresponding
RemoveEmpty*StaffContext statements quite easily in??
hara-kiri-percent-repeat.ly, but I'll need to add three new tests,
hara-kiri-drums.ly, hara-kiri-rhythmicstaves.ly and hara-kiri-tabs.ly
How about just writing a new regtest remove-empty-staff-context.ly that
demonstrates the removal of all of the staffs?
Drat! By the time I got your mail I'd already
done the mods and updated them for review. The tests I've written all
show the \RemoveEmpty*Context functions doing their stuff, and the
hara-kiri-percent-repeat one shows they all respond the same way to the
\repeat n percent command.
It would have been neater to go with your suggestion but I thought the
reg tests had to test a single thing per file.
They're up on Rietveld http://codereview.appspot.com/164045/show for
review.
Cheers,
Ian
|