You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-dev@hadoop.apache.org by "Doug Cutting (JIRA)" <ji...@apache.org> on 2007/03/22 21:21:32 UTC
[jira] Created: (HADOOP-1148) re-indent all code
re-indent all code
------------------
Key: HADOOP-1148
URL: https://issues.apache.org/jira/browse/HADOOP-1148
Project: Hadoop
Issue Type: Improvement
Reporter: Doug Cutting
Assigned To: Doug Cutting
Priority: Minor
We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Owen O'Malley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490180 ]
Owen O'Malley commented on HADOOP-1148:
---------------------------------------
+1
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: checkstyle-errors.html, indents2.patch, whitespace.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Raghu Angadi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489259 ]
Raghu Angadi commented on HADOOP-1148:
--------------------------------------
Doug,
Could you attach your indentation settings on your editor (emacs?)?
Does some one have config file for eclipse that sets the same settings?
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Nigel Daley <nd...@yahoo-inc.com>.
+1 to Doug's comments.
On Mar 22, 2007, at 2:03 PM, Doug Cutting (JIRA) wrote:
>
> [ https://issues.apache.org/jira/browse/HADOOP-1148?
> page=com.atlassian.jira.plugin.system.issuetabpanels:comment-
> tabpanel#action_12483321 ]
>
> Doug Cutting commented on HADOOP-1148:
> --------------------------------------
>
>> is there a consensus in this developer community that two spaces
>> are better than four?
>
> To make a change, we'd need a consensus that four are better than
> two. Two has been the stated goal since the project's inception.
>
> I prefer two because it permits more code per line while staying
> with 80 columns and using descriptive identifiers. I always edit
> in 80-column windows. Four spaces per indent level can quickly use
> nearly half the columns, with nested classes, anonymous methods,
> try/catch blocks, etc. It's nice when, e.g., 3-parameter method
> calls, where the method name and each parameter name might have ten
> characters, can be written on a single line, without wrapping each
> parameter to a separate line. I've never found two-space
> indentation to make code any less readable.
>
>> re-indent all code
>> ------------------
>>
>> Key: HADOOP-1148
>> URL: https://issues.apache.org/jira/browse/
>> HADOOP-1148
>> Project: Hadoop
>> Issue Type: Improvement
>> Reporter: Doug Cutting
>> Assigned To: Doug Cutting
>> Priority: Minor
>>
>> We should re-indent all code to consistently use 2-spaces per
>> level. This will not invalidate outstanding patches: one can use
>> the '-l' option to ignore whitespace differences in patches.
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483321 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
> is there a consensus in this developer community that two spaces are better than four?
To make a change, we'd need a consensus that four are better than two. Two has been the stated goal since the project's inception.
I prefer two because it permits more code per line while staying with 80 columns and using descriptive identifiers. I always edit in 80-column windows. Four spaces per indent level can quickly use nearly half the columns, with nested classes, anonymous methods, try/catch blocks, etc. It's nice when, e.g., 3-parameter method calls, where the method name and each parameter name might have ten characters, can be written on a single line, without wrapping each parameter to a separate line. I've never found two-space indentation to make code any less readable.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Konstantin Shvachko (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483319 ]
Konstantin Shvachko commented on HADOOP-1148:
---------------------------------------------
I agree we should re-indent uniformly to 2 spaces.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491383 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
Did you try changing patch's fuzz factor?
Another thing that sometimes works is to revert your workspace to a version where this patch applies, apply the patch, then use 'svn up' to sync with trunk. Subversion's internal patch system is generally better than patch.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: checkstyle-errors.html, indents2.patch, whitespace.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting resolved HADOOP-1148.
----------------------------------
Resolution: Fixed
Fix Version/s: 0.13.0
I just re-indented all the .java files, in revision 529410.
Patches can still be applied by using the -l flag, however the patched code will then need to be re-indented before it is committed. It's easy for me to automatically re-indent code with emacs, so if you have a patch that you'd like re-indented, I'd be happy to do that; just ask.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Raghu Angadi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491373 ]
Raghu Angadi commented on HADOOP-1148:
--------------------------------------
I am merging a largish patch made on 2-3 weeks old trunk. I think these changes are resulting in some rejections even with '{{patch -l}}'. For e.g. I could not find any other reason for following rejection for [FSDataset.java|http://svn.apache.org/viewvc/lucene/hadoop/trunk/src/java/org/apache/hadoop/dfs/FSDataset.java?view=log].
{noformat}
Index: src/java/org/apache/hadoop/dfs/FSDataset.java
===================================================================
--- src/java/org/apache/hadoop/dfs/FSDataset.java (revision 527216)
+++ src/java/org/apache/hadoop/dfs/FSDataset.java (working copy)
FSDataset.java.rej:
***************
*** 471,488 ****
/**
* Get a stream of data from the indicated block.
*/
- public synchronized InputStream getBlockData(Block b) throws IOException {
if (! isValidBlock(b)) {
throw new IOException("Block " + b + " is not valid.");
}
// File should be opened with the lock.
- return new FileInputStream(getFile(b));
}
/**
* Start writing to a block file
*/
- public OutputStream writeToBlock(Block b) throws IOException {
//
// Make sure the block isn't a valid one - we're still creating it!
//
--- 487,513 ----
/**
* Get a stream of data from the indicated block.
*/
+ public synchronized File getBlockFile(Block b) throws IOException {
if (! isValidBlock(b)) {
throw new IOException("Block " + b + " is not valid.");
}
// File should be opened with the lock.
+ return getFile(b);
}
+ static class BlockWriteStreams {
+ OutputStream dataOut;
+ OutputStream checksumOut;
+
+ BlockWriteStreams( File f ) throws IOException {
+ dataOut = new FileOutputStream( f );
+ checksumOut = new FileOutputStream( getMetaFile( f ) );
+ }
+ }
/**
* Start writing to a block file
*/
+ public BlockWriteStreams writeToBlock(Block b) throws IOException {
//
// Make sure the block isn't a valid one - we're still creating it!
//
{noformat}
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: checkstyle-errors.html, indents2.patch, whitespace.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Arun C Murthy (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483507 ]
Arun C Murthy commented on HADOOP-1148:
---------------------------------------
As much as I'd love to have two spaces everywhere I'm worried about the adverse impact on 'svn annotate' ... there, that's my best impression of the devil's advocate.
If everyone does agree on doing it anyway, I'd argue this is the right time to fix the line-lengths and also impose stricter rules on coding-conventions and adoption of tools like checkstyle in one shot.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Raghu Angadi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489484 ]
Raghu Angadi commented on HADOOP-1148:
--------------------------------------
True. I like th emacs argument alignment... but could not easily find where to set that in eclipse. I usually manually set it.
Also either in emacs or eclipse, I manually override this alignment to keep the text with in 80 columns. I think in your automated conversion some of these have crossed the 80 columns.
I think in patch submission, the enforcement should be semi-strict and code should not be auto indented.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489263 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
(add-hook 'find-file-hooks 'turn-on-auto-fill) ; always use fill
(add-hook 'text-mode-hook 'turn-on-auto-fill) ; always use fill
(defun my-code-mode-hook ()
(setq fill-column 79)
(setq comment-column 50)
(setq c-basic-offset 2)
(setq indent-tabs-mode nil) ; untabified indenting
)
(add-hook 'lisp-mode-hook 'my-code-mode-hook)
(add-hook 'c++-mode-hook 'my-code-mode-hook)
(add-hook 'java-mode-hook 'my-code-mode-hook)
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483364 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
> Does this issue aim to address this issue too?
No. Line-breaking would have a bigger affect on patches. Perhaps we might do that separately, but, for now, I was just thinking of re-indenting.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Tom White (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483593 ]
Tom White commented on HADOOP-1148:
-----------------------------------
+1
My feeling is that we should fix compile warnings (HADOOP-958) before fixing style warnings. However, this one is relatively unobtrusive, since there is a way of not invalidating current patches, so I would be happy for it to go before or in parallel with fixing warnings.
The checkstyle build target will check identation (as well as lots of other things), and I would encourage developers and committers to use it, especially for large chunks of new code. (I should confess at this point that I have yet to start using it in earnest as a part of the commit cycle!)
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "dhruba borthakur (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483344 ]
dhruba borthakur commented on HADOOP-1148:
------------------------------------------
I like the idea of having 80 column line width. Makes code easily readable. But some portions of existing code have longer line lengths. Does this issue aim to address this issue too?
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Arun C Murthy (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483821 ]
Arun C Murthy commented on HADOOP-1148:
---------------------------------------
> That is indeed the biggest cost that I can see. To find out who last edited a line one would have to use 'svn annotate -r XXX' where XXX is the last revision before the re-indentation. So there is a workaround. Is that acceptable?
Fair enough, I guess we have to bite the bullet at some point...
I'd like to propose we put up a reasonable svn 'tag' *before* (and after?) making this change so as to ease finding out the last revision i.e. help ease the process of finding the 'last revision before this change'. Does that sound reasonable? (I'm assuming svn has this feature, having used this in cvs)
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483697 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
> As much as I'd love to have two spaces everywhere I'm worried about the adverse impact on 'svn annotate'
That is indeed the biggest cost that I can see. To find out who last edited a line one would have to use 'svn annotate -r XXX' where XXX is the last revision before the re-indentation. So there is a workaround. Is that acceptable?
> I'd argue this is the right time to fix the line-lengths
That's more intrusive, as it would break outstanding patches. It's also harder to do well automatically.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Raghu Angadi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489298 ]
Raghu Angadi commented on HADOOP-1148:
--------------------------------------
Thanks. I think default behavior of 2 space indentation in one editor won't be same as 2 space indentation in another editor. Are those differences ok or will all new patches be re-indented by these settings?
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Hadoop QA (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489363 ]
Hadoop QA commented on HADOOP-1148:
-----------------------------------
Integrated in Hadoop-Nightly #60 (See http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Nightly/60/)
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting updated HADOOP-1148:
---------------------------------
Attachment: indents2.patch
Somehow yesterday I didn't manage to re-indent quite all the files. I'm not sure what happened. I've attached a patch of what would happen if I re-indent things now. Should I go ahead and do this?
Some files are really badly indented, others only indent parameters in the "eclipse" style, described above.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: indents2.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483701 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
> My feeling is that we should fix compile warnings (HADOOP-958) before fixing style warnings.
I don't feel strongly about the ordering. Fixing those compilation warnings is a manual process, no? And it may break existing patches. So it is heavier weight, but I don't see why it must happen first.
In both cases, a patch may be impractical, as it might soon become outdated. These are cases where, once we reach agreement, a committer might simply make and commit the change.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "David Bowen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12483311 ]
David Bowen commented on HADOOP-1148:
-------------------------------------
I agree that consistency in this regard would be good.
I don't want to start an argument over something so unimportant, but I was just wondering - is there a consensus in this developer community that two spaces are better than four?
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489456 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
> I think default behavior of 2 space indentation in one editor won't be same as 2 space indentation in another editor.
Right. The biggest variation seems to be around parameter indentation, whether a call is indented as: {{
foo(a,
b,
c);
}} as by emacs, or as {{
foo(a,
b,
c);
}}
as by eclipse.
Owen says he knows a way to get eclipse to use the former. I prefer the former, but would certainly not bounce a patch for using the latter. I'm not sure which the style checker prefers, if any.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489462 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
Lemme try that again, with _fancy_ *wiki* +formatting+ ??enabled??:
... whether a call is indented as: {noformat}
foo(a,
b,
c);
{noformat} as by emacs, or as something like {noformat}
foo(a,
b,
c);
{noformat}as by eclipse.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Tom White (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490171 ]
Tom White commented on HADOOP-1148:
-----------------------------------
+1
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: checkstyle-errors.html, indents2.patch, whitespace.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489503 ]
Doug Cutting commented on HADOOP-1148:
--------------------------------------
bq. I manually override this alignment to keep the text with in 80 columns.
A better way to fix that, if no other options are available, is to put the newline before the parenthesis:{noformat}
aVeryVeryLongVariableName.aVeryVeryLongMethodName
(firstVeryVeryLongParameterName,
secondVeryVeryLongParameterName,
thirdVeryVeryLongParameterName);
{noformat}
bq. I think in your automated conversion some of these have crossed the 80 columns.
Yes, but I don't think we should fix 80-column violations wholesale, since that would break outstanding patches. With indentation-only changes, existing patches can still be easily applied.
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Doug Cutting <cu...@apache.org>.
Nigel Daley wrote:
> FWIW, running just this rule:
> <module name="Indentation">
> <property name="basicOffset" value="2" />
> <property name="caseIndent" value="2" />
> </module>
> with trunk still yields 1799 indentation errors.
I get 1644, but I see your point.
% grep indentation build/test/checkstyle-errors.html | wc -l
1644
From a non-scientific sample, it looks like at least some of these are
the parameter list indentation issue.
Doug
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Nigel Daley <nd...@yahoo-inc.com>.
I think we should just have the automated patch process check for
style rules that we've already fixed. This should prevent regressions.
Then the nightly build can run checkstyle to report all the style
errors still existing in trunk.
FWIW, running just this rule:
<module name="Indentation">
<property name="basicOffset" value="2" />
<property name="caseIndent" value="2" />
</module>
with trunk still yields 1799 indentation errors.
Nige
On Apr 17, 2007, at 3:12 PM, Doug Cutting wrote:
> Nigel Daley wrote:
>> Hmm, we're still at almost 9000 style errors. I think it will be
>> too difficult for people to figure out where their problems are.
>> Perhaps I start by having the patch process generate the
>> checkstyle html file which can be looked at by committers. Thoughts?
>
> Sure, that'd be good. But patches still shouldn't increase the
> number.
>
> Looking at the current output, the most common things seem to be:
>
> % grep '^<td>' < build/test/checkstyle-errors.html | grep -v href |
> sed 's|<td>[0-9]*</td>$||' | sed 's|<[/]*td>||g' | sort | uniq -c |
> sort -nr | head -20
> 1321 '(' is followed by whitespace.
> 1299 ')' is preceded with whitespace.
> 1120 Line is longer than 80 characters.
> 477 First sentence should end with a period.
> 475 'if' construct must use '{}'s.
> 215 Redundant 'public' modifier.
> 196 ',' is not followed by whitespace.
> 168 '{' should be on the previous line.
> 139 Array brackets at illegal position.
> 123 method def modifier at indentation level 4 not at correct
> indentation, 2
> 121 ';' is preceded with whitespace.
> 119 Line contains a tab character.
> 118 Must have at least one statement.
> 117 '!' is followed by whitespace.
> 90 method def child at indentation level 6 not at correct
> indentation, 4
> 77 '}' should be on the same line.
> 52 method def rcurly at indentation level 4 not at correct
> indentation, 2
> 52 if child at indentation level 8 not at correct indentation, 6
> 51 Inner assignments should be avoided.
> 49 ';' is followed by whitespace.
>
> The first two of these concern whitespace, and could be fixed
> automatically w/o breaking patches.
>
> Doug
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Doug Cutting <cu...@apache.org>.
Raghu Angadi wrote:
> > 1321 '(' is followed by whitespace.
> > 1299 ')' is preceded with whitespace.
>
> Can we know how strict the policy here is? I surely contributed to quite
> a few of the above. The code just looks more readable in many cases.
The rule is "Sun's conventions except two-spaces per indent level".
Sun's conventions don't explicitly address this case, but all their
examples do not include spaces after '(' or before ')'.
http://java.sun.com/docs/codeconv/html/CodeConventions.doc7.html#475
> Initially I thought we want 2 space indentation. But these kinds of
> checks are a LOT more than that. I am not arguing about whether such a
> strict requirement is good or bad. I mainly want to know if that is the
> explicit policy.
The explicit policy of record is stated above. We could elaborate that,
or modify it to instead say "the following checkstyle settings"...
Doug
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Raghu Angadi <ra...@yahoo-inc.com>.
> 1321 '(' is followed by whitespace.
> 1299 ')' is preceded with whitespace.
Can we know how strict the policy here is? I surely contributed to quite
a few of the above. The code just looks more readable in many cases.
Initially I thought we want 2 space indentation. But these kinds of
checks are a LOT more than that. I am not arguing about whether such a
strict requirement is good or bad. I mainly want to know if that is the
explicit policy.
thanks,
Raghu.
Doug Cutting wrote:
> Nigel Daley wrote:
>> Hmm, we're still at almost 9000 style errors. I think it will be too
>> difficult for people to figure out where their problems are. Perhaps
>> I start by having the patch process generate the checkstyle html file
>> which can be looked at by committers. Thoughts?
>
> Sure, that'd be good. But patches still shouldn't increase the number.
>
> Looking at the current output, the most common things seem to be:
>
> % grep '^<td>' < build/test/checkstyle-errors.html | grep -v href | sed
> 's|<td>[0-9]*</td>$||' | sed 's|<[/]*td>||g' | sort | uniq -c | sort -nr
> | head -20
> 1321 '(' is followed by whitespace.
> 1299 ')' is preceded with whitespace.
> 1120 Line is longer than 80 characters.
> 477 First sentence should end with a period.
> 475 'if' construct must use '{}'s.
> 215 Redundant 'public' modifier.
> 196 ',' is not followed by whitespace.
> 168 '{' should be on the previous line.
> 139 Array brackets at illegal position.
> 123 method def modifier at indentation level 4 not at correct
> indentation, 2
> 121 ';' is preceded with whitespace.
> 119 Line contains a tab character.
> 118 Must have at least one statement.
> 117 '!' is followed by whitespace.
> 90 method def child at indentation level 6 not at correct
> indentation, 4
> 77 '}' should be on the same line.
> 52 method def rcurly at indentation level 4 not at correct
> indentation, 2
> 52 if child at indentation level 8 not at correct indentation, 6
> 51 Inner assignments should be avoided.
> 49 ';' is followed by whitespace.
>
> The first two of these concern whitespace, and could be fixed
> automatically w/o breaking patches.
>
> Doug
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Doug Cutting <cu...@apache.org>.
Nigel Daley wrote:
> Hmm, we're still at almost 9000 style errors. I think it will be too
> difficult for people to figure out where their problems are. Perhaps I
> start by having the patch process generate the checkstyle html file
> which can be looked at by committers. Thoughts?
Sure, that'd be good. But patches still shouldn't increase the number.
Looking at the current output, the most common things seem to be:
% grep '^<td>' < build/test/checkstyle-errors.html | grep -v href | sed
's|<td>[0-9]*</td>$||' | sed 's|<[/]*td>||g' | sort | uniq -c | sort -nr
| head -20
1321 '(' is followed by whitespace.
1299 ')' is preceded with whitespace.
1120 Line is longer than 80 characters.
477 First sentence should end with a period.
475 'if' construct must use '{}'s.
215 Redundant 'public' modifier.
196 ',' is not followed by whitespace.
168 '{' should be on the previous line.
139 Array brackets at illegal position.
123 method def modifier at indentation level 4 not at correct
indentation, 2
121 ';' is preceded with whitespace.
119 Line contains a tab character.
118 Must have at least one statement.
117 '!' is followed by whitespace.
90 method def child at indentation level 6 not at correct
indentation, 4
77 '}' should be on the same line.
52 method def rcurly at indentation level 4 not at correct
indentation, 2
52 if child at indentation level 8 not at correct indentation, 6
51 Inner assignments should be avoided.
49 ';' is followed by whitespace.
The first two of these concern whitespace, and could be fixed
automatically w/o breaking patches.
Doug
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Nigel Daley <nd...@yahoo-inc.com>.
Hmm, we're still at almost 9000 style errors. I think it will be too
difficult for people to figure out where their problems are. Perhaps
I start by having the patch process generate the checkstyle html file
which can be looked at by committers. Thoughts?
Nige
On Apr 17, 2007, at 2:02 PM, Doug Cutting wrote:
> Nigel Daley wrote:
>> So now that code indents are fixed, should we run checkstyle as
>> part of the patch process and -1 patches that increase the number
>> of style errors?
>
> +1
>
> Doug
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Doug Cutting <cu...@apache.org>.
Nigel Daley wrote:
> So now that code indents are fixed, should we run checkstyle as part of
> the patch process and -1 patches that increase the number of style errors?
+1
Doug
Re: [jira] Commented: (HADOOP-1148) re-indent all code
Posted by Nigel Daley <nd...@yahoo-inc.com>.
So now that code indents are fixed, should we run checkstyle as part
of the patch process and -1 patches that increase the number of style
errors?
Cheers,
Nige
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Tom White (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489529 ]
Tom White commented on HADOOP-1148:
-----------------------------------
Yes - please go ahead and finish the job. (BTW I've just committed HADOOP-1190 unchecked warnings changes for dfs but they don't conflict with your patch.)
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: indents2.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Hadoop QA (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12489691 ]
Hadoop QA commented on HADOOP-1148:
-----------------------------------
Integrated in Hadoop-Nightly #61 (See http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Nightly/61/)
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: indents2.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HADOOP-1148) re-indent all code
Posted by "Doug Cutting (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Doug Cutting updated HADOOP-1148:
---------------------------------
Attachment: checkstyle-errors.html
whitespace.patch
There were some problems with my earlier Emacs indentation scripting. It missed files that define multiple classes, and sometimes mis-indented methods with annotations.
I also:
- modified checkstyle's case indentation rule to use Sun's conventions;
- disabled checkstyle for JavaCC-generated files
- removed spaces before and after parens
- removed spaces before semicolons
- added spaces after commas
The number of checkstyle warnings are now radically reduced, with very few now that can be fixed by changing spacing alone (so that patches survive).
1115 Line is longer than 80 characters.
474 First sentence should end with a period.
414 'if' construct must use '{}'s.
215 Redundant 'public' modifier.
135 Array brackets at illegal position.
117 Line contains a tab character.
116 Must have at least one statement.
101 ',' is not followed by whitespace.
56 '}' should be on the same line.
40 '{' should be on the previous line.
38 ';' is not followed by whitespace.
34 'else' construct must use '{}'s.
33 'private' modifier out of order with the JLS suggestions.
28 '(' should be on the previous line.
27 'static' modifier out of order with the JLS suggestions.
24 Utility classes should not have a public or default constructor.
23 '{' is followed by whitespace.
22 'name' hides a field.
21 'for' construct must use '{}'s.
20 Inner assignments should be avoided.
Should I commit this?
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: checkstyle-errors.html, indents2.patch, whitespace.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HADOOP-1148) re-indent all code
Posted by "Hadoop QA (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HADOOP-1148?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490332 ]
Hadoop QA commented on HADOOP-1148:
-----------------------------------
Integrated in Hadoop-Nightly #64 (See http://lucene.zones.apache.org:8080/hudson/job/Hadoop-Nightly/64/)
> re-indent all code
> ------------------
>
> Key: HADOOP-1148
> URL: https://issues.apache.org/jira/browse/HADOOP-1148
> Project: Hadoop
> Issue Type: Improvement
> Reporter: Doug Cutting
> Assigned To: Doug Cutting
> Priority: Minor
> Fix For: 0.13.0
>
> Attachments: checkstyle-errors.html, indents2.patch, whitespace.patch
>
>
> We should re-indent all code to consistently use 2-spaces per level. This will not invalidate outstanding patches: one can use the '-l' option to ignore whitespace differences in patches.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.