Letting this patch wait for issue 5708 to catch up. No merge of the two issues is necessary; I'll just push them concurrently. --- Wait, that's nonsense. The regtest cannot work without the feature being present. Han-Wen, do you want to add the Rietveld patch for 5708 to the review of this one? Otherwise I'd have to create a new Rietveld issue containing both since Rietveld issues can only be changed by their owner.
👍
1
Last edit: David Kastrup 2020-01-29
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I didn't see the conversation here, and thought we were OK with doing an example in another commit per discussion on the code review. Should I revert the commit?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Ah ok. No need to revert, but please add a test as David said (I thought issue 5708 already had one, but I misread the thread. Sorry for the accusation.)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Ah ok. No need to revert, but please add a test as David said (I thought
issue 5708 already had one, but I misread the thread. Sorry for the
accusation.)
--- old+++ new@@ -1,3 +1,7 @@This will allow certain user-defined engravers to do useful things
https://codereview.appspot.com/561310045
++-----++Regression test: https://codereview.appspot.com/557380044/
Patch: waiting --> new
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
--- old+++ new@@ -1,7 +1,3 @@This will allow certain user-defined engravers to do useful things
https://codereview.appspot.com/561310045
---------Regression test: https://codereview.appspot.com/557380044/
status: Started --> Fixed
Patch: new -->
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
passes make, make check and a full make doc.
Patch on countdown for Jan 29th
Patch counted down - please push.
Do we really want to push this without the reg test? Why is this a separate issue in https://sourceforge.net/p/testlilyissues/issues/5708/?
I propose to merge these two and put this back on review.
Letting this patch wait for issue 5708 to catch up. No merge of the two issues is necessary; I'll just push them concurrently. --- Wait, that's nonsense. The regtest cannot work without the feature being present. Han-Wen, do you want to add the Rietveld patch for 5708 to the review of this one? Otherwise I'd have to create a new Rietveld issue containing both since Rietveld issues can only be changed by their owner.
Last edit: David Kastrup 2020-01-29
Exactly. I'm not sure though if Han-Wen saw your question, AFAICT there's no notification for edits.
Han-Wen, why is commit 1a75cad4c7e79c3630ca9d393152c61786eebd36 in
masterwithout a regression test?I didn't see the conversation here, and thought we were OK with doing an example in another commit per discussion on the code review. Should I revert the commit?
Not putting it in 2.20 anyway. Just make sure to followup soon so that the example/test makes it into 2.21. Thanks!
Ah ok. No need to revert, but please add a test as David said (I thought issue 5708 already had one, but I misread the thread. Sorry for the accusation.)
https://codereview.appspot.com/557380044/ for a test.
this took a bit, as some of this code did change since the last time I saw
it ;-)
On Mon, Feb 10, 2020 at 8:06 PM Jonas Hahnfeld hahnjo@users.sourceforge.net
wrote:
Related
Issues:
#5688Diff:
Putting this back to review so James can track the regression test.
Diff:
Ah sorry, this is issue 5747.