Posted by sun on October 1, 2011 at 8:09pm:
Problem
- Uncertainty about when or how to apply coding standards across Drupal core versions and core vs. contrib code.
Goal
-
Clarify the scope of coding standards with regard to:
- major versions of core
- core vs. contrib code
-
Explicitly specify/state which particular coding standards have been different for earlier versions.
Details
-
In #1290258-59 some (legit) confusion arose around my statement in a contrib issue:
Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be. Especially for larger code-bases (like Drupal core), updating the code of a previous version for current standards may too huge of a task. However, code in current versions should follow the current standards. Lastly, when following this practice, make sure to not squeeze coding standards updates/clean-ups into otherwise unrelated patches.
-
Based on that, the main people caring for Drupal coding standards discussed it and added the following intro text to http://drupal.org/coding-standards:
Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be. Especially for larger code-bases (like Drupal core), updating the code of a previous version for the current standards may too huge of a task. However, code in current versions should follow the current standards.
Note: Do not squeeze coding standards updates/clean-ups into otherwise unrelated patches. Only touch code lines that are actually relevant. To update existing code for the current standards, always create separate and dedicated issues and patches.
Based on the confusion that arose in aforementioned issue, this could have probably been discussed some more. ;)
-
As determined in aforementioned issue, there are coding standards - e.g., for string concatenation - which we currently apply differently for Drupal core patches depending on the core version a patch is for.
I.e., patches backported from D7 to D6 may be adjusted for the differing string concatenation rules being used throughout Drupal 6 core for consistency.
(Note, however, that I (sun) am currently not sure whether this "should" is actually turned into a "required" for backported D6 patches.)
-
The above coding standards intro text was primarily intended to clarify the scope of current coding standards in contributed extensions, and also clarify how to update (or ignore) existing code.
Proposed resolutions (and their drawbacks)
- Require all code to meet version-specific standards.
-
We have no currently-published version-specific standards, and no easy way to produce them.
- Require all code to be consistent with the surrounding code, regardless of standards.
-
The surrounding code, despite huge efforts at standardization, often lacks consistency.
- Require all code to meet currently-published standards, regardless of surrounding code.
-
This introduces inconsistencies which standards are written to avoid.
- Require all code to pass coder review as written for the target branch of core.
-
Coder review needs more active maintainers before this option can be considered.
- Use a common-sense approach.
-
Even people who have "common sense" often disagree on what it means.
Discuss at http://drupal.org/node/1296842.
Comments
New coding standards being applied to old code
I am having trouble with getting my project application approved due to some confusion with the application of coding standards. I have been amending the code quite rigourously since March, to comply with coding standards and take in to account reviewer's suggestions.
The issue arose when a reviewer used the D7 version of Coder to automatically review my module for D6. Because I had been using the Coder module for D6, I had not seen many of the errors that the reviewer posted. Furthermore, some of the coding standards that I had been working with, have changed since I started the project application process. It seems quite possible that I will never satisfy coding standards if I take too much time in amending my code. For a large module such as the one I have been coding, this is a big burden.
The issue where this is discussed can be seen here:
http://drupal.org/node/1110538#comment-5217782
This statement seems sound:
However, in the project review process, it is not immediately obvious what code is new and what code is old - all contributed code there appears to be treated as "new", when this is not necessarily the case. This has the affect of delaying previously compliant code from being approved as new standards are applied.
In this situation, it seems to make sense that the coding standards in the latest version of Coder for the version of Drupal for which the module is written should be the ones applied.
If not, then perhaps reviewers should be made aware that new coding standards do not need to be complied with for older code.
Any comments will be welcome.
IMO
From my perspective, any code in the project review process is new code, until it is promoted to a full project. However, from an 'approval' perspective, if the only issues remaining are a small number of trivial coding standard violations, then reviewers have been more than willing to mark projects RTBC despite the remaining issues.
In the particular case referenced in your link, the script also flagged a number of 'critical' issues, which on their own would block the application with or without coding standard violations. Rather than simply stating 'I've considered and they are not an issue', I'd suggest addressing each one in turn, and explaining in the queue why they are not an issue; which saves the reviewer from having to go back and look at the code, perform their own analysis, and then come to that same conclusion on their own.
Coder
I have not yet had time to look closer at the coder internals. But is it possible, that both the D6 and the D7 release are supplied with the same (i.e. D7 related) rules while they should actually check the standards that is published in the documentation for each major version?
And/or: Is it possible that some reviewers check D6 projects with a D7 version of coder? This seems to be the case many times, looking at the many complaints about "Implementation of hook_xy" vs. "Implements hook_xy()"?
I would see a big profit for both reviewers and applicants in clarifying this point!
I have already stated that in the linked discussion: If I can help with e.g. updating the coding standards documents (if necessary) or something else related to this process, I will be glad to do that.
Coding standards *are* version-dependent.
See #1296842-29 and #711918-145:
Also see #1161796-7:
So I was right?
So the practice of using D7 coder module for D6 contrib code is already considered bad practice?
Doesn't that suggest that the following statement on the coding standards page is incorrect?
Shouldn't it be:
...confused...
Also, should this conversation be had on this page or this one?: http://drupal.org/node/1296842
"standards"
Tommy,
I would leave http://drupal.org/node/1296842 alone to focus on Drupal core, while this discussion is dealing primarily with contrib modules.
We have been working towards automating portions of the project application process. I do not anticipate that anyone involved really wants to try and extend these automation efforts to take drupal core version into account and then apply a different coding style ruleset to the code based on that core version.
So while historically, some may say that coding standards are version dependent; I anticipate that the model forward for the project applications process will be to use a single (i.e. the most current) version of the coding standards when evaluating future modules ... and the current version will be whatever ruleset is flagged by the automated tools at that time. What doesn't change is the practice of not enforcing backporting of coding standard changes ... the standards on the day of that review become the standards which are evaluated against for that project.
I'm assuming this discussion has come up regarding the 'Implementation of hook_foo().' versus 'Implements hook_foo().' line items flagged by the PAReview script. This was a coding standard change during the transition from Drupal 6 to Drupal 7, and a very common indicator flagged by the script (which uses D7 Coder) when evaluating Drupal 6 modules. You are correct that typical Drupal 6 pattern (recommendation?) was to use the 'Implementation of hook_foo().' wording ... and the 'new' standard is to use the 'Implements hook_foo().' version.
I appreciate that you may have started your code before this change in the coding standards. That said, the main purpose of established coding standards is to drive consistency in code ... and we also have a desire to provide a higher level of consistency in our project application reviews. Our intent is to simplify the human review process as much as possible, and applying two different coding standards based on core version runs contrary to this goal.
With this in mind, I would suggest that the practice of using D7 coder module for D6 contrib code in the project application queue is not bad practice ... it is the current practice; and I anticipate it will remain the ongoing practice in the future.
Is it wrong to use 'Implementation of'?
No.
Is it difficult to perform a global search and replace to change this to 'Implements'?
No.
Does debating the point really make the best use of our time?
... probably not.
Hey jthorson, I agree and
Hey jthorson, I agree and also see more sense in the approach to add new code agreeing to the new standards. As I already wrote, I simply would like to minimize the confusion towards new applicants as I am, so we should do everything to make "clear orders" in the documents.
Just to get this really clear to myself: As long as I do not use klausi's shell script for pre-reviews of my own stuff, would you then recommend me checking my D6 projects also within a D7 environment? And, to avoid confusion as I and surely some other newbies just faced it recently, shouldn't that be clearly pointed out in the documents?
Also I think you got us wrong when you ask for the sense of "debating" (I agree there is none), as that definitely isn't the point here. It is all about clarification for the benefit of all, especially those coming new into the community. It is all about making the best of our all time, just as you imply!
So please be indulgent with us new folks here. All we want is to contribute, not to dispute (at least I suppose that). ;)
... would you then recommend
Just as we don't want to force reviewers through any unnecessary hoops, we certainly don't want to force applicants through any unnecessary hoops ... I think you're fine running through D6 coder, if that's the environment you have. As long as there are no other issues identified in the review, I don't think anyone is going to block an application strictly for coding standards issues; especially in the case of D6 to D7 changes such as 'Implements'.
The key, however, is that whichever coding standard you have adopted needs to be applied consistently. If your code uses a mix of "Implements" and "Implementation of", or uses spaces around your assignment operators in some cases and not in others, the application will probably be bounced - especially when the two cases occur on the same line! ;)
I certainly understand where coding standards starts to feel like a lot of 'nit-picking' ... especially when a project has been sitting in the queue for a very long time. It can be extremely frustrating to be sent back and told to go fix your spacing 6 months after submitting your application. This is one of the reasons we're actively working to change the process, to make it less painful for new applicants in the future ... but the changes tend to be rather dynamic as we try new things, and the documentation tends to lag behind the implementation.
In the end, conversation, contribution, and dispute are all healthy parts of this process, so long as they occur in a polite and objective manner. :)
Hi and thanks again, since I
Hi and thanks again, since I have enough server space under my direction I can run as many environments as I like (within certain borders, that is) ;) That said and repeating that I am really keen on producing code that meets the current standards: Is it better to check my code inside a D7 coder implementation then?
And, by the way, I am a nit-picker myself regarding processes and standards. I even earn substantial parts of my wages with similar things ;).
Regarding the lags, I stick to my offer to help out e.g. in documentation, as far as you old stagers would consider it making sense to have that done by a newbie.
Most important to me is, however, returning something into the entire project as I have drawn a lot of benefit out of it for myself :)
If your reviewer is using
If your reviewer is using PAReview.sh, then a D7 Coder implementation would give equivalent results ... but there is no requirement for a reviewer to use the script. Other reviewers may run D6 Coder during reviews instead. So there isn't a straight-forward answer ...
... others might suggest you should be building your modules for D7 instead of D6, and the answer becomes much clearer! ;)
Ok, nice to see that this
Ok, nice to see that this community appears to be a genuine multicultural melting pot. ;)
BTW, "my reviewer"? I have a dedicated one? Cool, who is it? ;)
On to acting...
Thanks for your input on this jthorson, I feel cleverer as a result.
I think it would be helpful for reviewers to be able to consider the points that we have talked about. The best place to do it is probably How to review a Full Project Application - Goals.
I would suggest changing:
To:
[EDIT] On second thoughts, looking at your other posts, I would guess that you would be against this. It's a difficult balance to strike.