Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(150)

Issue 347000043: Improve markup->string

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 4 months ago by thomasmorley651
Modified:
5 years, 4 months ago
Reviewers:
dak, pwm
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Issue 5437 Improve markup->string 'all-relevant-markup-commands' is now a toplevel-defined procedure. So it is not longer a part of the rekursive 'markup->string'. It needs to be a procedure, because not all bindings of the lily-module are already done in markup.scm, so it should be evaluated at the time 'markup->string' is called. Additionally we gain the chance to have user-defined markup-commands from '(current-module)' been processed as well. Formerly uncatched markup-commands with string?-predicate are special-cased: wordwrap-string-markup, justify-string-markup and simple-markup. Others are added to 'markup-commands-to-ignore'

Patch Set 1 #

Patch Set 2 : simplify, catch string-markups #

Patch Set 3 : simplify, catch string-markups #

Total comments: 4

Patch Set 4 : Paul's review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -25 lines) Patch
M scm/markup.scm View 1 2 3 4 chunks +41 lines, -25 lines 0 comments Download

Messages

Total messages: 11
thomasmorley651
Please review. There are some TODOs in the code where I'd appreciate some feedback. Thanks-
5 years, 4 months ago (2018-11-10 12:50:29 UTC) #1
dak
I wonder whether it might be reasonable to just have all markup commands with a ...
5 years, 4 months ago (2018-11-10 12:53:18 UTC) #2
thomasmorley651
On 2018/11/10 12:53:18, dak wrote: > I wonder whether it might be reasonable to just ...
5 years, 4 months ago (2018-11-11 10:57:31 UTC) #3
dak
On 2018/11/11 10:57:31, thomasmorley651 wrote: > On 2018/11/10 12:53:18, dak wrote: > > I wonder ...
5 years, 4 months ago (2018-11-11 11:07:35 UTC) #4
thomasmorley651
On 2018/11/11 11:07:35, dak wrote: > On 2018/11/11 10:57:31, thomasmorley651 wrote: > > 'all-relevant-markup-commands' tries ...
5 years, 4 months ago (2018-11-11 12:40:05 UTC) #5
thomasmorley651
simplify, catch string-markups
5 years, 4 months ago (2018-11-11 15:05:55 UTC) #6
thomasmorley651
simplify, catch string-markups
5 years, 4 months ago (2018-11-11 15:08:57 UTC) #7
pwm
Hi Harm, I took a quick look and it LGTM at a quick read. I ...
5 years, 4 months ago (2018-11-11 15:43:37 UTC) #8
thomasmorley651
On 2018/11/11 15:08:57, thomasmorley651 wrote: > simplify, catch string-markups patch-set 3 reflects my musing in ...
5 years, 4 months ago (2018-11-11 15:53:42 UTC) #9
thomasmorley651
Paul's review
5 years, 4 months ago (2018-11-11 15:56:17 UTC) #10
thomasmorley651
5 years, 4 months ago (2018-11-11 15:57:42 UTC) #11
Hi Paul,

thanks for review

https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm
File scm/markup.scm (right):

https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm#newcode141
scm/markup.scm:141: ;; The string is split at line-breaks, emty strings removed
and finally
On 2018/11/11 15:43:37, pwm wrote:
> typo: empty

Done.

https://codereview.appspot.com/347000043/diff/40001/scm/markup.scm#newcode148
scm/markup.scm:148: simple-markup)))
On 2018/11/11 15:43:37, pwm wrote:
> Indent 'list' expression so it is clearer that it is the second argument to
> 'member' and not another 'and' expression.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b