Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document whitespace policy, provide editor configurations for non-Python source files #21793

Closed
mkoeppe opened this issue Nov 1, 2016 · 25 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Nov 1, 2016

This is a follow-up on #21792.

CC: @embray @dimpase @jdemeyer

Component: scripts

Author: Matthias Koeppe

Branch/Commit: a23bca3

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/21793

@mkoeppe mkoeppe added this to the sage-7.5 milestone Nov 1, 2016
@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 1, 2016

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 1, 2016

comment:2

Needs review or contributions regarding other editors.


New commits:

4052874Add Emacs configuration - no tabs for indentation
cb55eb7Add whitespace policy

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 1, 2016

Commit: cb55eb7

@jhpalmieri
Copy link
Member

comment:3

You have to allow tabs in makefiles, so the Emacs configuration should probably apply (indent-tabs-mode . nil) specifically for Python files, Cython files, shell scripts, ReST files, txt files. Any others? (Emacs should recognize a shell script and set the appropriate mode from the line #!/usr/bin/env bash. My copy of Emacs doesn't recognize the file build/make/deps as being a Makefile, and note that this file contains tabs.)

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 1, 2016

comment:4

Emacs already overrides a globally set indent-tabs-mode in makefile-mode.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

e38309eSet Emacs file mode

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2016

Changed commit from cb55eb7 to e38309e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

1d37130Override indent-tabs-mode for Makefiles

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2016

Changed commit from e38309e to 1d37130

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 1, 2016

comment:7

But you are right, the directory-local setting was overriding the setting from makefile-mode. Fixed.

@jhpalmieri
Copy link
Member

comment:8

This will change the behavior of Emacs on build/make/deps unless the user specifically sets makefile-mode when opening that file. I don't think that's a good idea.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 1, 2016

comment:9

Replying to @jhpalmieri:

This will change the behavior of Emacs on build/make/deps unless the user specifically sets makefile-mode when opening that file. I don't think that's a good idea.

Did you see the change that sets the major mode of that file?

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 1, 2016

Author: Matthias Koeppe

@jhpalmieri
Copy link
Member

comment:11

Replying to @mkoeppe:

Replying to @jhpalmieri:

This will change the behavior of Emacs on build/make/deps unless the user specifically sets makefile-mode when opening that file. I don't think that's a good idea.

Did you see the change that sets the major mode of that file?

No, but I see it now.

@jdemeyer
Copy link

jdemeyer commented Nov 4, 2016

comment:13

I don't like how this becomes the first section of the "General conventions" document, as if it is the most important thing. I would for example put it right before the section "The Pickle Jar".

Also the title "General Code Style" doesn't really convey that it's about whitespace.

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 6, 2016

comment:14

Doesn't it belong into the chapter "General Conventions"? Perhaps last section of it?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 6, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

68137f4Move whitespace section just before pickle jar section

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 6, 2016

Changed commit from 1d37130 to 68137f4

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 6, 2016

comment:16

Replying to @mkoeppe:

Doesn't it belong into the chapter "General Conventions"? Perhaps last section of it?

Please disregard.

I agree with you. Please see the current patch.

@jdemeyer
Copy link

jdemeyer commented Nov 7, 2016

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

jdemeyer commented Nov 7, 2016

comment:17

If you checked that the documentation actually builds, you can set this to positive_review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2016

Changed commit from 68137f4 to a23bca3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 7, 2016

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3443b24Add Emacs configuration - no tabs for indentation
13d4f47Add whitespace policy
b578e49Set Emacs file mode
2886c37Override indent-tabs-mode for Makefiles
a23bca3Move whitespace section just before pickle jar section

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 7, 2016

comment:19

It does build (just verified again after rebasing to current beta). Thanks.

@vbraun
Copy link
Member

vbraun commented Dec 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants