Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

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

Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

nine.fierce.ballads
Reviewers: ,

Message:
Regtest differences attached to the ticket are expected.  I would
appreciate independent confirmation; is anyone interested in taking a
little time to understand the case? After that, I think it will make
sense to push this, since the code was reviewed last week and is just
enabled by this patch.

Description:
https://sourceforge.net/p/testlilyissues/issues/5736/

Enable the previously reviewed block of code required to fix this
regression test.

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

Affected files (+1, -2 lines):
  M lily/context.cc


Index: lily/context.cc
diff --git a/lily/context.cc b/lily/context.cc
index 4fe1652751f9862908e75ce20642e6db3dc8fa1d..2a2871f32b5dbbd36afe4b0828e7e3d25138df0f 100644
--- a/lily/context.cc
+++ b/lily/context.cc
@@ -181,8 +181,7 @@ Context::unchecked_find (FindMode mode, Direction dir,
   const bool allow_create = (mode != FIND_ONLY);
   const bool allow_find = (mode != CREATE_ONLY);
 
-  // TODO: Enabling this block will fix input/regression/context-find-parent.ly.
-  if (false && allow_find && (dir == CENTER))
+  if (allow_find && (dir == CENTER))
     {
       // Search everything in and below the scope of the current context first.
       // Here is an example that depends on finding a context below.



Reply | Threaded
Open this post in threaded view
|

Re: Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

Han-Wen Nienhuys-3
can you add a pointer to the commit that introduces the previous code?
I'm in the middle of a laptop switch , so I can't look it up right now.

https://codereview.appspot.com/559460043/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

nine.fierce.ballads
In reply to this post by nine.fierce.ballads
On 2020/02/06 09:59:26, hanwenn wrote:
> can you add a pointer to the commit that introduces the previous code?
I'm in
> the middle of a laptop switch , so I can't look it up right now.

I've added this link to the description of the current review.
https://codereview.appspot.com/579250043/diff/565550049/lily/context.cc#newcode184


https://codereview.appspot.com/559460043/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by nine.fierce.ballads
LGTM

nit: please use the commit SHA1 for referencing previous code. It is
self-contained, and doesn't need the internet for understanding the
context.

https://codereview.appspot.com/559460043/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

Han-Wen Nienhuys-3
In reply to this post by nine.fierce.ballads
(why is this review closed?)

https://codereview.appspot.com/559460043/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

nine.fierce.ballads
In reply to this post by nine.fierce.ballads
On 2020/02/07 11:27:56, hanwenn wrote:
> (why is this review closed?)

Jonas reviewed the results and I pushed the change and closed the
review.

https://codereview.appspot.com/559460043/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

David Kastrup
In reply to this post by Han-Wen Nienhuys-3
[hidden email] writes:

> LGTM
>
> nit: please use the commit SHA1 for referencing previous code. It is
> self-contained, and doesn't need the internet for understanding the
> context.

For referencing previous issues, we have so far used the issue number,
so that would be rather something to discuss on the list rather than as
a side note.

We have been able to transport the issue database across several
different systems, so the issue numbers have proven a comparatively
workable reference frame.  Commit ids work just in Git.

For information strictly related to a single commit, like when reverting
a commit, a commit id is certainly a good reference but at least in the
case of reverts, the issue number contained in the original commit
message is also present.

In addition, issue numbers tend to be associated with reports showing
the history and rationale behind a change and often giving extensive
other information like problem images.  That's something we cannot
attach to commits (or at least, this has not been attempted so far).

--
David Kastrup

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5736: Fix input/regression/context-find-parent.ly (issue 559460043 by nine.fierce.ballads@gmail.com)

Han-Wen Nienhuys-3
On Fri, Feb 7, 2020 at 3:44 PM David Kastrup <[hidden email]> wrote:

> > nit: please use the commit SHA1 for referencing previous code. It is
> > self-contained, and doesn't need the internet for understanding the
> > context.
>
> For referencing previous issues, we have so far used the issue number,
> so that would be rather something to discuss on the list rather than as
> a side note.
>
>
I'll do that.

--
Han-Wen Nienhuys - [hidden email] - http://www.xs4all.nl/~hanwen