Re: [PATCH 3/4] Make define-builtin-markup{, -list}-command #:category #:properties keywords

[Carl Sorensen]

On 11/23/09 2:05 PM, "David Kastrup" <address@hidden> wrote:

> Nicolas Sceaux <address@hidden> writes:
> 
>> Le 23 nov. 2009 à 01:03, David Kastrup a écrit :
>> 
>>> The specification of category and properties makes the *-builtin-*
>>> variants diverge syntactically from the user specified markup.  Moving
>>> those specifications into keyword arguments makes the builtin defining
>>> macros upwards compatible with the user specified ones.
>> 
>> Could you publish your patches on retvield?
>> It's just a matter of git-cl upload from your branch.
> 
> Nope.  The contributor's guide says:
> 
>        Then, add the git-cl directory to your PATH, or create a symbolic
>     link to the git-cl and upload.py in one of your PATH directories (like
>     usr/bin).  git-cl will is then configured by
> 
>          git-cl config
> 
>     and answering the questions that are asked.
> 
> There is no clue just _how_ one should answer the questions or what they
> mean.  Let's see where we go:
> 
> $ git-cl config
> Rietveld server (host[:port]) [codereview.appspot.com]:
> CC list: address@hidden
> Tree status URL:
> ViewVC URL:
> 
> address@hidden:/usr/local/tmp/lilypond$ git-cl status
> Branches associated with reviews:
>       master: None
> 
> Current branch: no issue assigned.

Since you haven't uploaded a patch, you don't have any reviews assigned yet.

Did you try git-cl upload?

> 
> $
> 
> Does not look really convincing.
> 
> 
> address@hidden:/usr/local/tmp/lilypond$ git-cl rebase
> Can't locate SVN/Core.pm in @INC (@INC contains: /opt/git/share/perl/5.10.0
> /etc/perl /usr/local/lib/perl/5.10.0 /usr/local/share/perl/5.10.0
> /usr/lib/perl5 /us
> 
> 
> Really, that's not something that would appear to helpmy workflow.
> 
> 
> git-checkout -d mad-dak
> 
> then piping the four postings through git-am should do the trick for
> reviewing.  Feel free to post the thing to Rietveld yourself if you feel
> like it, but at the moment I really can't be bothered fighting some
> none-working tools.
> 
>> I think somewhere you missed a change from
>> define-builtin-markup-list-command to define-markup-list-command.
> 
> I did not change any define-builtin-markup-list-command to
> define-markup-list-command in this patch series.
> 
>> It would be easier to read and comment on retvield, together with
>> other modified files.
> 
> Feel free to put it there.
> 
>> It also seems that ly/markup-init.ly is impacted, but I don't see the
>> patch.
> 
> It isn't impacted.  The patch series gives the *-builtin-* commands a
> different syntax that is upwards-compatible to the user counterparts.
> It does not change the user counterparts at all.
> 
> The first two patches in the series are cleanups and would work
> independently of the rest of the patch series.  I had posted them
> previously on the list and they have basically been ignored.  They are
> in the patch series because they touch code _overlapping_ with patch 4,
> and it makes no sense in not doing the cleanup.
> 
> Patch 3 actually changes changes the *-builtin-* syntax and consequently
> renders Lilypond inoperative when applied alone.  That is the core of
> the patch series, and it is much better reviewable when kept separate
> from patch 4.

Not if you're using Reitveld with side-by-side diffs.  The two patches
together are very nicely reviewable.

IIUC, our policy is that *every* patch that is applied should result in a
buildable LilyPond.  If not, it's a bad patch.

Thanks,

Carl