Ready lilypond-mode for packaging for emacs 24 package system, such that lilypond-mode will work wi…

Ready lilypond-mode for packaging for emacs 24 package system, such that lilypond-mode will work with Melpa.

[pschmied, 2014-12-23 01:21:48]

Hi all,

I was hoping to create a lilypond-mode package to load into emacs via the
excellent and popular Melpa package repository. I had done an initial packaging,
however the Melpa maintainer has noted some issues with the lilypond-mode
distributed with lilypond. I've done my best at trying to address those, and to
ready lilypond-mode for said packaging. Please see this thread for a discussion
of those changes:

https://github.com/milkypostman/melpa/pull/2301

Cheers,
Peter

[pkx166h, 2014-12-26 17:02:12]

On 2014/12/23 01:21:48, pschmied wrote:
> Hi all,
> 
> I was hoping to create a lilypond-mode package to load into emacs via the
> excellent and popular Melpa package repository. I had done an initial
packaging,
> however the Melpa maintainer has noted some issues with the lilypond-mode
> distributed with lilypond. I've done my best at trying to address those, and
to
> ready lilypond-mode for said packaging. Please see this thread for a
discussion
> of those changes:
> 
> https://github.com/milkypostman/melpa/pull/2301
> 
> Cheers,
> Peter

Patch on countdown for Dec 29th

[dak, 2014-12-26 18:53:51]

https://codereview.appspot.com/191260043/diff/1/elisp/lilypond-mode.el
File elisp/lilypond-mode.el (right):

https://codereview.appspot.com/191260043/diff/1/elisp/lilypond-mode.el#newcod...
elisp/lilypond-mode.el:112: (progn
This progn is unnecessary.  The old code gets an OMG from me.

https://codereview.appspot.com/191260043/diff/1/elisp/lilypond-mode.el#newcod...
elisp/lilypond-mode.el:1165: (add-hook 'LilyPond-mode-hook (lambda ()
(turn-on-font-lock)))
(lambda () (turn-on-font-lock)) should just be #'turn-on-font-lock instead.

But font-lock-mode is a global preference.  I don't think that modes should
unconditionally enable it.

[dak, 2014-12-29 11:22:46]

Is somebody actually feeling responsible for this issue?  Comments have not been
adressed, but it would appear that both Rietveld issue and Google issue have
been created by the same person, so he should have seen the comments?

[pkx, 2014-12-29 12:44:19]

David,

On 29/12/14 11:22, dak@gnu.org wrote:
> Is somebody actually feeling responsible for this issue?  Comments have
> not been adressed, but it would appear that both Rietveld issue and
> Google issue have been created by the same person, so he should have
> seen the comments?
> 
> https://codereview.appspot.com/191260043/

Well as you'll see I did reply via the Reitveld, I do this often for
those random patches. I just cannot add him as an owner in the Tracker.

So don't worry, as long as there is a tracker item I will make sure it
is properly processed and if I have any doubts I will 'shout' to the group.

I've a few errands to do today, but I am due a patch countdown at some
point this afternoon and so this Rietveld/Tracker will be updated
accordingly.

Regards

James

[pschmied, 2014-12-29 15:39:40]

Hi all,

Is there something that I need to do at this point in the system? I see the
comment about the unnecessary `progn` in the code. Should I nix that and
resubmit?

Cheers,
Peter


On Mon, Dec 29, 2014 at 4:44 AM, James Lowe <pkx@gnu.org> wrote:

> David,
>
> On 29/12/14 11:22, dak@gnu.org wrote:
> > Is somebody actually feeling responsible for this issue?  Comments have
> > not been adressed, but it would appear that both Rietveld issue and
> > Google issue have been created by the same person, so he should have
> > seen the comments?
> >
> > https://codereview.appspot.com/191260043/
>
> Well as you'll see I did reply via the Reitveld, I do this often for
> those random patches. I just cannot add him as an owner in the Tracker.
>
> So don't worry, as long as there is a tracker item I will make sure it
> is properly processed and if I have any doubts I will 'shout' to the group.
>
> I've a few errands to do today, but I am due a patch countdown at some
> point this afternoon and so this Rietveld/Tracker will be updated
> accordingly.
>
> Regards
>
> James
>

[dak, 2014-12-29 16:21:25]

On 2014/12/29 15:39:40, pschmied wrote:
> Hi all,
> 
> Is there something that I need to do at this point in the system? I see the
> comment about the unnecessary `progn` in the code. Should I nix that and
> resubmit?

The progn is a style issue but not a show stopper.  However, unconditionally
enabling font-lock-mode is inappropriate for a mode: this should be fixed and
resubmitted, and while you are at it, removing the progn seems reasonable.

It has the advantage of getting back at the original indentation and thus a
smaller diff.

Incidentally, it would seem that you have pulled the kill-buffer inside of the
while loop, a really bad idea.  Without the progn, you'd likely have noticed
this change.

[pschmied, 2014-12-29 17:41:13]

Oops. Good catches. I'll make those edits and resubmit today.

On Mon, Dec 29, 2014 at 8:21 AM, <dak@gnu.org> wrote:

> On 2014/12/29 15:39:40, pschmied wrote:
>
>> Hi all,
>>
>
>  Is there something that I need to do at this point in the system? I
>>
> see the
>
>> comment about the unnecessary `progn` in the code. Should I nix that
>>
> and
>
>> resubmit?
>>
>
> The progn is a style issue but not a show stopper.  However,
> unconditionally enabling font-lock-mode is inappropriate for a mode:
> this should be fixed and resubmitted, and while you are at it, removing
> the progn seems reasonable.
>
> It has the advantage of getting back at the original indentation and
> thus a smaller diff.
>
> Incidentally, it would seem that you have pulled the kill-buffer inside
> of the while loop, a really bad idea.  Without the progn, you'd likely
> have noticed this change.
>
> https://codereview.appspot.com/191260043/
>

[pkx166h, 2015-01-01 23:19:43]

Just for the record this is logged as 

https://code.google.com/p/lilypond/issues/detail?id=4230

James

[pschmied, 2015-01-05 18:12:18]

Address code review comments regarding readying lilypond-mode for melpa
packaging.

[pschmied, 2015-01-05 18:16:01]

On 2015/01/05 18:12:18, pschmied wrote:
> Address code review comments regarding readying lilypond-mode for melpa
> packaging.

Hmm... The patchset didn't seem to upload. Sorry about the spam, everyone. Still
trying to figure out git-cl, and what incantation will upload a patchset.

[pschmied, 2015-01-05 18:17:04]

Attempt 2 at uploading revised patch set to ready lilypond-mode for melpa
packaging.

[pschmied, 2015-01-09 17:46:53]

On 2015/01/05 18:17:04, pschmied wrote:
> Attempt 2 at uploading revised patch set to ready lilypond-mode for melpa
> packaging.

Closing this issue, as James has generously offered to help me with the proper
patch updating / formatting, which I seem to be deficient at doing.