Use directory-local variables to establish some coding styles in Emacs

Use directory-local variables to establish some coding styles in Emacs

[Ian Hulin (gmail), 2012-08-19 12:12:36]

Do we need to add anything to the standard .gitignore for these?

They're files in the git directories for developers developing the kit rather
than users writing code.

Quite a lot of developers have the build directory within the git tree.

There are already .el files in the kit for lilypond mode in emacs.

Is this an issue for *this* patch?

Just some issues possibly coming in from the blind side. 

Cheers,
Ian

[dak, 2012-08-19 12:37:38]

On 2012/08/19 12:12:36, Ian Hulin (gmail) wrote:
> Do we need to add anything to the standard .gitignore for these?

Why should we?  This adds just one file which _should_ be checked out.

> They're files in the git directories for developers developing the kit rather
> than users writing code.

If a developer wants to add his directory local variables, he'll add his own
.gitignore entry.

> Quite a lot of developers have the build directory within the git tree.

So?

> There are already .el files in the kit for lilypond mode in emacs.

What is "the kit"?

> Is this an issue for *this* patch?

Is _what_ an issue for this patch?

> Just some issues possibly coming in from the blind side. 

I don't get the point you are trying to make.

[Graham Percival, 2012-08-19 16:02:31]

Could you add a script (either shell or python) which applies this formatting to
all scheme files in the repository?  I'd like to see how that changes the scheme
files.

http://codereview.appspot.com/6460109/diff/3001/.dir-locals.el
File .dir-locals.el (right):

http://codereview.appspot.com/6460109/diff/3001/.dir-locals.el#newcode6
.dir-locals.el:6: (indent-tabs-mode))
Does this mean what I think it means?  We should have *no* tabs in C++ files
(decided in last year's GOP).

This year's GOP question proposes to extend that to no tabs for C++ and scheme
files.  I haven't heard any disagreement with that part of the proposal, so
please speak up if you do.

I'm pretty certain that the CG on docs specifies to use only spaces in those
files.  This is important for formatting @lilypond  for html and probably LaTeX
as well.

[Ian Hulin (gmail), 2012-08-19 16:07:28]

On 2012/08/19 12:37:38, dak wrote:
> On 2012/08/19 12:12:36, Ian Hulin (gmail) wrote:
> > Do we need to add anything to the standard .gitignore for these?
> 
> Why should we?  This adds just one file which _should_ be checked out.
> 
> > They're files in the git directories for developers developing the kit
rather
> > than users writing code.
> 
> If a developer wants to add his directory local variables, he'll add his own
> .gitignore entry.
> 
> > Quite a lot of developers have the build directory within the git tree.
> 
> So?
> 
> > There are already .el files in the kit for lilypond mode in emacs.
> 
> What is "the kit"?
> 
> > Is this an issue for *this* patch?
> 
> Is _what_ an issue for this patch?
> 
> > Just some issues possibly coming in from the blind side. 
> 
> I don't get the point you are trying to make.

Sorry for being obtuse.  I'd like your patch description to say more and give
answers to some of the following questions.

Who is the target audience for this patch, LilyPond developers or end users
using emacs to write their scores? Or both?

Where is your new .dir-locals.el going in the git directory tree? (Hence the
question about .gitignore.  I've just started using emacs and there was tip from
the web to allow you to specify what directories built-in compile and gdb used
as their pwd. This involved adding or customizing a .dire-locals.el.)  

Where is it going to end up after a make install run and where will it be
delivered when the binaries are built and run?

Is it a file users could or should be able to customize for their emacs
sessions? If it is, add some dire comment block warnings saying "Don't change
this bit".

Implementing coding styles for LilyPond developers is *a good thing*. Adding
some automation of those styles is also a good thng.  Well done for taking this
on, just make it a bit clearer to people not so heavily involved with emacs what
your doing and what you want to achieve.

Cheers,
Ian Hulin

[dak, 2012-08-19 16:37:42]

http://codereview.appspot.com/6460109/diff/3001/.dir-locals.el
File .dir-locals.el (right):

http://codereview.appspot.com/6460109/diff/3001/.dir-locals.el#newcode6
.dir-locals.el:6: (indent-tabs-mode))
On 2012/08/19 16:02:31, Graham Percival wrote:
> Does this mean what I think it means?

No, it doesn't.  This is an association list written out by Emacs'
add-dir-local-variable command.  If it would have been written by hand, this
line would better have looked like
  (indent-tabs-mode . nil)
In Lisp, nil serves all the purposes that '() _and_ #f serve
in Scheme.  So this line turns _off_ indent-tabs-mode.  Turning it _on_ in this
context would have looked like
  (indent-tabs-mode . t)
instead.  That (indent-tabs-mode) is a rather confusing print form for
(indent-tabs-mode . nil) might be one reason that Scheme decided to have a
"false" boolean separate from '().

> We should have *no* tabs in C++ files
> (decided in last year's GOP).

[Further complaint elided for brevity]

Substituting the equivalent longer form for the sake of human readability is not
likely a good idea since every additional call of add-dir-local-variable will
again rewrite it in the short form.  Similarly for adding a manual comment.

You'll just have to live with it.  This file is not really intended for human
consumption, anyway.

[dak, 2012-08-19 16:50:54]

On 2012/08/19 16:07:28, Ian Hulin (gmail) wrote:
> On 2012/08/19 12:37:38, dak wrote:
>
> > I don't get the point you are trying to make.
> 
> Sorry for being obtuse.  I'd like your patch description to say more and give
> answers to some of the following questions.
> 
> Who is the target audience for this patch, LilyPond developers or end users
> using emacs to write their scores? Or both?

Huh?  Like everything written in the git tree, the "target audience" are
LilyPond developers.

> Where is your new .dir-locals.el going in the git directory tree?

Huh?  This is a patch to be reviewed.  It is going exactly where the patch says
it does, in the top directory of the LilyPond repository.

> Where is it going to end up after a make install run and where will it be
> delivered when the binaries are built and run?

Huh?!?!?!?  It has nothing to do whatsoever with "make install".  It is a single
file sitting in the top working directory of the git tree.

It sets defaults for editing files with Emacs inside of the work tree directory
hierarchy.

> Is it a file users could or should be able to customize for their emacs
> sessions? If it is, add some dire comment block warnings saying "Don't change
> this bit".

Why?  It is a file you can change if you want to, like any other file checked
into the git repository.

> Implementing coding styles for LilyPond developers is *a good thing*. Adding
> some automation of those styles is also a good thng.  Well done for taking
this
> on, just make it a bit clearer to people not so heavily involved with emacs
what
> your doing and what you want to achieve.

I don't understand the problem.  If someone wants a different commit message,
I'll be happy to replace it.

[Graham Percival, 2012-08-19 17:06:20]

On Sun, Aug 19, 2012 at 04:50:54PM +0000, dak@gnu.org wrote:
> On 2012/08/19 16:07:28, Ian Hulin (gmail) wrote:
> >Where is it going to end up after a make install run and where will it
> be
> >delivered when the binaries are built and run?
> 
> Huh?!?!?!?  It has nothing to do whatsoever with "make install".  It is
> a single file sitting in the top working directory of the git tree.

If that file was inside elisp/  (which I agree that we can see
from Rietveld that it is not), then I would expect that it would
be automatically installed.

> >Who is the target audience for this patch, LilyPond developers or end
> users
> >using emacs to write their scores? Or both?
> 
> Huh?  Like everything written in the git tree, the "target audience" are
> LilyPond developers.

I think that Ian's confusion is understandable.  Files inside
elisp/*.el are mostly not intended for developers.  At least,
stuff like elisp/lilypond-mode.el seems to be aimed directly at
end-users, not developers.


David, please calm down.  We want to encourage people to review
patches.  The tone of your replies to Ian is not encouraging to
reviewers.  I agree that Ian has misunderstood a few things, but
these should be explained politely.  The use of multiple question
marks and exclamation marks is not polite.

- Graham

[Ian Hulin (gmail), 2012-08-19 17:19:54]

Hi David,

I thought I'd been polite in framing my questions.  Statements like
"Huh?" and "Huh?!?!?!?" come over as very rude.  If you're puzzled why
I'm asking them, just say so.

See below for specifics.

On 19/08/12 17:50, dak@gnu.org wrote:
> On 2012/08/19 16:07:28, Ian Hulin (gmail) wrote:
>> On 2012/08/19 12:37:38, dak wrote:
>
>> > I don't get the point you are trying to make.
>
>> Sorry for being obtuse.  I'd like your patch description to say more
> and give
>> answers to some of the following questions.
>
>> Who is the target audience for this patch, LilyPond developers or end
> users
>> using emacs to write their scores? Or both?
>
> Huh?  Like everything written in the git tree, the "target audience" are
> LilyPond developers.
>
Then say so in your commit message.
>> Where is your new .dir-locals.el going in the git directory tree?
>
> Huh?  This is a patch to be reviewed.  It is going exactly where the
> patch says it does, in the top directory of the LilyPond repository.
>
>> Where is it going to end up after a make install run and where will it
> be
>> delivered when the binaries are built and run?
>
> Huh?!?!?!?  It has nothing to do whatsoever with "make install".  It is
> a single file sitting in the top working directory of the git tree.
Wasn't  clear from Rietveld display, my bad.

Make install moves things around from from the git tree to the places
like locations the local machine's /usr... tree.
If it's a new file, its a valid question to ask where it will end up
once a user installs LilyPond on his/her machine.
Make it clear in the commit message.
>
> It sets defaults for editing files with Emacs inside of the work tree
> directory hierarchy.
>
Say so in the commit message.
>> Is it a file users could or should be able to customize for their
> emacs
>> sessions? If it is, add some dire comment block warnings saying "Don't
> change
>> this bit".
>
> Why?  It is a file you can change if you want to, like any other file
> checked into the git repository.
>
>> Implementing coding styles for LilyPond developers is *a good thing*.
> Adding
>> some automation of those styles is also a good thng.  Well done for
> taking this
>> on, just make it a bit clearer to people not so heavily involved with
> emacs what
>> your doing and what you want to achieve.
>
> I don't understand the problem.  If someone wants a different commit
> message, I'll be happy to replace it.
>
Yes please, (see above).


>
> http://codereview.appspot.com/6460109/

Cheers,

Ian

[John Mandereau, 2012-08-19 20:06:15]

Il giorno dom, 19/08/2012 alle 16.02 +0000, graham@percival-music.ca ha
scritto:
> Could you add a script (either shell or python) which applies this
> formatting to all scheme files in the repository?  I'd like to see how
> that changes the scheme files.

You can try running (after having applied David's patch)

emacs -batch scm/*.scm --eval '(progn (delete-trailing-whitespace)
(indent-region (point-min) (point-max) nil) (save-buffer))'

On my system this has only changed x11-color.scm.

Note that this doesn't remove tabs (although I installed David's patch
that adds .dir-locals.el), as you can notice with

git add scm
emacs -batch *.scm --eval '(progn (delete-trailing-whitespace) (indent-region
(point-min) (point-max) nil) (untabify (point-min) (point-max)) (save-buffer))'
git diff

which further changes x11-color.scm.

I attached the result of

git diff HEAD scm


If you're fine with the above emacs call (the one with untabify of course) going
in
scripts/auxiliar/fixscm.sh, I'll submit a new patch to take over this
issue.

Best,
John