[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Carl Sorensen
Subject: Re: [PATCH 3/4] Make define-builtin-markup{, -list}-command #:category #:properties keywords
Date: Mon, 23 Nov 2009 16:04:05 -0700

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 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]) []:
> 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/ 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/ 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]