Project

General

Profile

Actions

Bug #14246

closed

Inconsistent C source code indentation

Added by shyouhei (Shyouhei Urabe) over 4 years ago. Updated about 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:84498]

Description

I would like to focus on following 238 C source codes in our repository.

% git ls-files | grep '\.\(c\|h\|def\)$' | grep -v 'ext|spec|test' | wc -l
238

Here, in these 238 files, 10 files are indented using spaces only.

% git ls-files | grep '\.\(c\|h\|def\)$' | grep -v 'ext|spec|test' | \
xargs grep --files-without-match $'^\t' | xargs grep --files-with-match '^        ' | wc -l
10

On the other hand 66 files are indented using tabs.

% git ls-files | grep '\.\(c\|h\|def\)$' | grep -v 'ext|spec|test' | \
xargs grep --files-with-match $'^\t' | xargs grep --files-without-match '^        ' | wc -l
66

Other 61 files do not indent. We should not bother them.

% git ls-files | grep '\.\(c\|h\|def\)$' | grep -v 'ext|spec|test' | \
xargs grep --files-without-match $'^\t' | xargs grep --files-without-match '^        ' | wc -l
61

So far so good. But what about remaining 101 files? The answer is obvious; these files MIX indents.

% git ls-files | grep '\.\(c\|h\|def\)$' | grep -v 'ext|spec|test' | \
xargs grep --files-with-match $'^\t' | xargs grep --files-with-match '^        ' | wc -l
101

This is totally wrong. No matter should we use spaces or tabs for indentations, it must be consistent.


Related issues 1 (0 open1 closed)

Related to Ruby master - Misc #16112: Reduce the possibility of "expand tabs" commit occurrencesClosedActions

Updated by duerst (Martin Dürst) over 4 years ago

I'm all for better consistency. I thought there were clear guidelines for Ruby, and (almost) everybody was following them. I didn't know it was that bad.

But fixing spacing will obscure where the code originally came from. I don't really like that.

To improve the situation, I have the following suggestions:

  1. Install an SVN bot that fixes indents on newly committed lines automatically. That would hopefully catch the attention of the committers.

  2. Based on svn blame, get a picture of who is how good (or not) at following the guidelines, and gently nudge everybody in the right direction.

[Somebody sooner or later will start discussion on what the best way of indenting should be. If you want to do this, please make it a separate issue. Please tag it as "joke" (because we don't have a "bikeshed" category).]

Actions #2

Updated by shyouhei (Shyouhei Urabe) over 4 years ago

  • Description updated (diff)

Updated by normalperson (Eric Wong) over 4 years ago

wrote:

I'm all for better consistency. I thought there were clear
guidelines for Ruby, and (almost) everybody was following
them. I didn't know it was that bad.

Right, I remember it being 4 spaces per-level, and use TAB
whenever it's 8 spaces. AFAIK, that's the Emacs default and I've
seen this in several other projects, so not uncommon..
In vim, I use: :set ts=8 sw=4 sts=4 noexpandtab

But fixing spacing will obscure where the code originally came
from. I don't really like that.

Agreed 100%. Noise makes code archaelogy harder, I don't like
whitespace-only changes; but I'd be happy to see them if one is
in the affected area and already changing code, and the proposed
SVN bot might help with that...

I admit that I moved some existing all-space code not long ago
and did not notice it, but nobu fixed it for me :x

To improve the situation, I have the following suggestions:

  1. Install an SVN bot that fixes indents on newly committed
    lines automatically. That would hopefully catch the attention
    of the committers.

Instead of auto-fixing, it should email the committer;
that ought to reduce future screwups.

Updated by znz (Kazuhiro NISHIYAMA) over 4 years ago

I think that guidelines are .editorconfig for most editors, misc/ruby-style.el for Emacs, and .indent.pro for indent.

Updated by vo.x (Vit Ondruch) over 4 years ago

Let me quote Developers How-To 1:

  • indent
  • 4 for C
  • 2 for Ruby
  • tab/space
  • Do not use TABs in ruby codes ruby-dev:19388
  • Use TAB instead of 8 SPs in C. (Emacs's default style)

Updated by graywolf (Gray Wolf) over 4 years ago

As @vo.x (Vit Ondruch) already pointed out, there are guidelines for the indentation. So based on the guidelines the 101 files are (probably) correctly indented and the 76 (10 + 66) files are (probably) wrongly indented.

Updated by shyouhei (Shyouhei Urabe) over 4 years ago

@graywolf (Gray Wolf) we are talking about C codes here. The "Do not use TABs" policy applies to non-C files.

Updated by graywolf (Gray Wolf) over 4 years ago

I can't count to 8 in

xargs grep --files-with-match '^        '`

, sorry, just ignore me. :/

Updated by shyouhei (Shyouhei Urabe) over 4 years ago

We discussed this issue in today's developer meeting.

  • We agreed not to batch update the indents at once. Indents should become consistent over time.
  • Matz has no strong opinion on this topic.
  • We agreed to move to spaces only. Reasons behind this:
    • No contemporary advantage of mixing tabs and spaces today.
    • Emacs fans at the meeting could use ruby-style.el, and that style can be updated.

So the next action is:

  • Modify ruby-style.el and other config files.
  • Use them.
Actions #11

Updated by Anonymous over 4 years ago

  • Status changed from Open to Closed

Applied in changeset trunk|r62033.


.editorconfig: Use spaces instead of tab except Makefiles

ref [Bug #14246]
[ci skip]

Updated by znz (Kazuhiro NISHIYAMA) over 4 years ago

  • Status changed from Closed to Open

Re-open because accidentally closed.
At least misc/ruby-style.el set to tabs.

Actions #13

Updated by k0kubun (Takashi Kokubun) over 4 years ago

  • Status changed from Open to Closed

Applied in changeset trunk|r62789.


misc/ruby-style.el: use spaces for indentation

instead of hard tabs.

[Bug #14246]

Actions #14

Updated by k0kubun (Takashi Kokubun) almost 3 years ago

  • Related to Misc #16112: Reduce the possibility of "expand tabs" commit occurrences added

Updated by ioquatix (Samuel Williams) about 2 months ago

Does this policy apply to the formatting of default gems too?

Updated by shyouhei (Shyouhei Urabe) about 2 months ago

ioquatix (Samuel Williams) wrote in #note-16:

Does this policy apply to the formatting of default gems too?

I don't think so. One good thing about splitting things into gems is they can have their own policies (release timing etc.). I support each gems having distinct point of view as to what code is a beautiful code.

Actions

Also available in: Atom PDF