Coding standards for file ending newlines
Dave Reid - October 12, 2008 - 01:21
| Project: | Coder |
| Version: | 7.x-1.x-dev |
| Component: | Review/Rules |
| Category: | task |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Novice |
Description
After a discussion in #drupal, decided to be consistant with trailing newlines in core files. Going to do a search-replace for '\W+\Z' in core and get a patch up.
#1
#2
#3
you will get a "no ending newline" from patch if that's missing imo... it does no harm and sometimes you need to fight your editor to not add one.
#4
Maybe we should consider making sure all core files have an ending newline? Getting the no ending newline patch warning is something that might confuse novices and make them think they had done something wrong (like I had for a while when I first started patching core).
If not, feel free to mark as won't fix again.
#5
The only file in a clean installation of HEAD matching the pattern '\W+\Z' is misc/jquery.js, but it has a trailing end line. Can we consider this fixed?
#6
While I'm at it, let's set the status to move this issue along.
Does someone want to confirm that all core files have a trailing newline?
#7
misc/jquery.js is the only one that comes up for me, too.
#8
That's no longer the pattern that needs to be checked. We want to ensure that all files in core end in exactly one new line, which matches several files.
#9
#10
Oops...left a spare part of a contact.module patch in there.
#11
Went through the first 15 or so changes and everything checked out. Patch applies cleanly and works fine. Follows standards. Don't really know how to review it except to say that :).
#12
The last submitted patch failed testing.
#13
Let's retest.
#14
The last submitted patch failed testing.
#15
Bump..
The patch no longer works due to all the intervening changes to the code. Is there some automated process to pick out all files in core that don't end in one new line and make them end in exactly one new line?
I'd reroll this if I knew how, but I suspect I'm going to waste a lot of time playing around with line endings...
#16
rerolled, temporarily critical, due to amount of changes.
this patch was made possible thanks to the song Code Monkey by Jonathan Coulton
http://www.jonathancoulton.com/2006/04/14/thing-a-week-29-code-monkey/
#17
#18
The last submitted patch failed testing.
#19
what happened bot?
- Failed to install HEAD
- WTF??? stupid bot!!!
#20
The last submitted patch failed testing.
#21
do not trust the bot: http://testing.drupal.org/node/33
#22
The last submitted patch failed testing.
#23
Setting to previous status - testbot was broken (failed to install).
#24
and it passes, testbot is not broken anymore :D
note that there are no new-line warnings in the adds, and all newline removals were duplicate newlines at the end of the file.
#25
Thanks for that.
How'd you find the files with newline problem? Was it just figuring out the right regex string?
#26
did not even used regex ;)
0) remove CVS and binary (images) files!!!
1) append unique identifier to all and every file
2) search and replace magic
3) create intermediate.patch
4) check out CVS again
5) apply intermediate patch to clean CVS checkout
6) no problems :D
7) cvs diff
8) patch ready!
#27
Oh. I see..
I was hoping for a quick way of doing it. =P Turns out, grep can't search across multiple lines anyway...
#28
The last submitted patch failed testing.
#29
This isn't critical.
I'm also heavily considering "won't fix"ing this. I don't think we'll ever reach a point where everyone's editors aren't (re-)introducing/stripping these trailing whitespaces, so this kind of feels like a fight we're never going to win.
#30
If we can get the core committers to run patches through some kind of automatic coder review, it won't matter anymore. That would be sweet. :) My original point is that newbies creating patches that don't have an ending new line are going to see those wacky lines in their patches and will freak out thinking they did it wrong.
#31
At least, we should add this to the coding standards.
/me hates missing newline at end of file
#32
sun: That sounds reasonable. Want to take care of it?
Dave: I know, but there are obviously editors out there that are stripping out trailing newlines, or this wouldn't come up all the time. And rather than core committers doing this, the testing bot should do this. Then it could be fixed way before it ever hits RTBC. I know there must be an issue for this somewhere but I can't find it.
#33
Moving to Coder's queue.
#34
@webchick: Oh yeah having it automated by the bot is a way better idea. I love it!
#35