Posted by doitDave on November 19, 2011 at 8:56pm
Currently the review process faces an issue that turns out really annoying.
TL; DR: The code checking scripts (pareview and drupalcs, which are really great besides!), complain when a file ends in \n but give green lights on \n\n. The docs read different. Currently this is more and more leading to disputes.
Can we solve this asap? Be it either that the documents are corrected or the review tools?
Related links collection:
- http://drupal.org/coding-standards#comment-5262644
- http://drupal.org/node/1340276#comment-5267276
- http://drupal.org/node/1339850#comment-5267124
- http://drupal.org/node/1328898#comment-5267298 (also several comments above this)
And more. Also tried to catch someone on IRC but all seemed afk (saturday night ftw).
Thanks!
Comments
Copying comments from 1328898
Some interpret 'end with a single newline' as there should be no more than one "\n" at the end of the file, and others interpret it as 'the end of a file (last line) should contain a single newline'. If we're talking core patches, then strictly the former is the correct interpretation - but I know that I personally always assumed the second.
Let's try not to hold up applications for basic coding standards violations ... this will get sorted out through automation as we turn it up (since a bot will only enforce one interpretation or the other) ... but for now, I don't really think it's worth holding up applications for this particular coding standards debate, which doesn't really affect readability of the code in any way. Point out the change, but if that's the only issue, let's not 'CNW' the application.
That said, I'd still block any files which don't have 'at least one' newline (due to the 'no newline at end of file' error that future patches would trigger), and no file should ever have 'more than two'.
All good
That sounds good.
The only reason of user is to avoid the no newline verbose message in a diff/patch. One newline or two, it doesn't matter, so long as there is one.
Yeah, thanks for these
Yeah, thanks for these points; I simply agree. :)
Can I suggest
That the tool http://ventral.org/pareview be set as the standard.
I've put in a request for this at: http://groups.drupal.org/node/195303
This new line issue is trivial - but can cause huge pain when people are trying to submit code and are getting reviews pointing out line errors which conflict with what their choice of tool is reporting.
bailey86, Patrick's online
bailey86, Patrick's online service cannot be defined as a standard, as it is "only" a bridge to the existing scripts available to all reviewers and, of course, all applicants.
I think what you refer to is that suddenly "new" errors appear with each review - which is not really true, as they only have not been revealed before but the tools have improved. (Let's not speak of false positives, which unfortunately happens as well.)