Problem/Motivation
We need a policy on when and/or how to break across lines function calls, function declarations, and language constructs that exceed 80 characters. In the absence of a policy, our code lacks uniformity, our patches are getting held up on discussions of opinion, and our automated code scanning tools offer counter-productive advice (see original report).
Goal
Define and document a policy and update our code scanning tools accordingly.
Proposed resolution: PSR-2 += as at #68
Rules defined by PSR are in italic, points from discussion are not. Examples for PSR rules can be found at https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-codi....
-
The soft limit on line length MUST be 120 characters; automated style checkers MUST warn but MUST NOT error at the soft limit.
Lines SHOULD NOT be longer than 80 characters; lines longer than that SHOULD be split into multiple subsequent lines of no more than 80 characters each.
Developers SHOULD focus on readability and understandability of the overall code logic for people who are reading the code for the first time or need to debug it.
-
When declaring functions, argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.
When the argument list is split across multiple lines, the closing parenthesis and opening brace MUST be placed together on their own line with one space between them.
-
Conditional control structures (
if
/else
/while
) SHOULD be written on a single line, using self-descriptive and readable parameters and expressions. However, they MAY be written in multi-line syntax, if that's best for the business logic being performed, and/or keeping the code readable and documented. In that case, use this format:if (!isset($file) // If no core compatibility version is defined at all, then it is // impossible to check for updates. || !isset($file->info['core']) // If the extension is not compatible with the current core, it cannot // be enabled in the first place. || $file->info['core'] != DRUPAL_CORE_COMPATIBILITY // If the new version requires a higher PHP version than the available, // then it is not possible to update to it. The PHP version property // defaults to DRUPAL_MINIMUM_PHP for all extensions. || version_compare(phpversion(), $file->info['php']) < 0) { return TRUE; }
I.e.: Developers MUST use another indentation level for the wrapping conditions in the multi-line syntax and SHOULD provide an inline comment for each condition.
Keeping the operators at the beginning of the line has two advantages: It is trivial to comment out a particular line during development while keeping syntactically correct code (except of course the first line). Further is the logic kept at the front where it's not forgotten. Scanning such conditions is very easy since they are aligned below each other.
-
When using explicitly declared values for control structures, and the declaration alone exceeds 80 chars, it is RECOMMENDED to move the declaration into a variable instead.
$numbers = array(1, 2, 3, 5, 7, 11, 13, 17, 19, 23, 1, 2, 3, 5, 7, 11, 13, 17, 19, 23); foreach ($numbers as $n) { drupal_set_message(t('%number is prime!', array('%number' => $n))); }
-
foreach
,for
control structures MUST NOT use multi-line syntax within their definition. E.g., do NOT do this:foreach (array( 1, 2, 3, 5, 7, 11, 13, 17, 19, 23, ) as $n) {
-
When calling functions, argument lists MAY be split across multiple lines, where each subsequent line is indented once. When doing so, the first item in the list MUST be on the next line, and there MUST be only one argument per line.
$foo = foo_function( $term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Translate some message here; good times.'), t('Some other message here.'), array( 'foo' => 'thing', 'bar' => 'thing2', ), $bar );
Parameters that contain natural language strings, references to values deeply nested within arrays, inline/"options-style" definitions of arrays or other parameters over 30 characters long are often good candidates for arguments to consider splitting across multiple lines.
Developers SHOULD focus on the readability of the logic of the entire code block though, and take into account how common the parameters of the called function are. As an example for the latter, which is passing forward the passed-in arguments only:
file_field_load($entity_type, $entities, $field, $instances, $langcode, $items, $age);
-
Values in an options-style associative array with more than one value SHOULD be wrapped onto multiple lines; e.g.:
$result = format_string('@foo at some point before had the value of @bar', array( '@foo' => $foo, '@bar' => t('The Bar in @foo', array( '@foo' => $foo, )), ));
-
A nested call to a callable returning an object with chainable methods SHOULD be placed on the same line, using the regular coding style for chained method calls on the new indentation level; e.g.:
$value = db_select('users') ->fields('users', array('uid')) ->condition('uid', $account->uid, '<>') ->condition(db_or() ->condition('mail', db_like($form_state['values']['mail']), 'LIKE') ->condition('name', db_like($form_state['values']['mail']), 'LIKE') ) ->range(0, 1) ->execute() ->fetchField();
-
Opening and closing parentheses MUST use the same level of indentation across multiple lines.
Proposed resolution as at #68
- Functional code MAY be wrapped where it improves overall readability. Unlike comments there is no specific per-line character limit for functional code, since the involved class and function names alone often consume more than 80 characters already. Developers SHOULD focus on readability and understandability of the overall code logic for people who are reading the code for the first time or need to debug it. Finding the right balance between verbosity of individual actions and the overall logic being performed in a code block REQUIRES human sense and evaluation and CANNOT be predetermined by coding standards.
- Function declarations MUST be written on a single line; they should never wrap multiple lines.
- Conditional control structures (
if
/else
/where
) SHOULD be written on a single line, using self-descriptive and readable parameters and expressions. However, they MAY be written in multi-line syntax, if that's best for the business logic being performed, and/or keeping the code readable and documented. In that case, use this format:
if (!isset($file) // If no core compatibility version is defined at all, then it is // impossible to check for updates. || !isset($file->info['core']) // If the extension is not compatible with the current core, it cannot // be enabled in the first place. || $file->info['core'] != DRUPAL_CORE_COMPATIBILITY // If the new version requires a higher PHP version than the available, // then it is not possible to update to it. The PHP version property // defaults to DRUPAL_MINIMUM_PHP for all extensions. || version_compare(phpversion(), $file->info['php']) < 0) { return TRUE; }
I.e.: Developers MUST use another indentation level for the wrapping conditions in the multi-line syntax and SHOULD provide an inline comment for each condition.
Keeping the operators at the beginning of the line has two advantages: It is trivial to comment out a particular line during development while keeping syntactically correct code (except of course the first line). Further is the logic kept at the front where it's not forgotten. Scanning such conditions is very easy since they are aligned below each other.
- When using explicitly declared values for control structures, and the declaration alone exceeds 80 chars, it is RECOMMENDED to move the declaration into a variable instead.
$numbers = array(1, 2, 3, 5, 7, 11, 13, 17, 19, 23, 1, 2, 3, 5, 7, 11, 13, 17, 19, 23); foreach ($numbers as $n) { drupal_set_message(t('%number is prime!', array('%number' => $n))); }
foreach
andfor
control structures MUST NOT use multi-line syntax within their definition. E.g., do NOT do this:
foreach (array( 1, 2, 3, 5, 7, 11, 13, 17, 19, 23, ) as $n) {
- Function call parameters MAY wrap multiple lines, with each parameter on its own line. This MAY increase code readability when the length of individual paramters is very long, especially when the function call includes other function calls or arrays. For example:
$foo = foo_function( $term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Translate some message here; good times.'), t('Some other message here.'), array( 'foo' => 'thing', 'bar' => 'thing2', ), $bar );
Ie. Parameters that contain natural language strings, references to values deeply nested within arrays, inline/"options-style" definitions of arrays or other parameters over 30 characters long are often good candidates to be considered "very long" parameters.
Developers SHOULD focus on the readability of the logic of the entire code block though, and take into account how common the parameters of the called function are. As an example for the latter, which is passing forward the passed-in arguments only:
file_field_load($entity_type, $entities, $field, $instances, $langcode, $items, $age);
Function calls MUST either wrap all parameters or none. Do NOT do this:
$foo = foo_function($term_1[$lancode][$delta]['value']['tid'], $term_2[$lancode][$delta]['value']['tid'], t('Message with %arg1 and @arg2', array( '%arg1' => $bar, '@arg2' => $baz, )));
- Values in an options-style associative array that contains more than one value SHOULD be wrapped onto multiple lines; e.g.:
$result = format_string('@foo at some point before had the value of @bar', array( '@foo' => $foo, '@bar' => t('The Bar in @foo', array( '@foo' => $foo, )), ));
- Parameters of nested function calls, MAY be wrapped onto multiple lines, and MAY keep the first parameter on the same line; e.g.:
$this->assertEqual( $article_built[$field_name]['#items'][0]['fid'], $default_images['field_new']->fid, format_plural($count, 'An existing article node without an image has the expected default image file ID of @fid.', '@count existing article nodes without an image have the expected default image file ID of @fid.', array( '@fid' => $default_images['field_new']->fid ) ) );
- A nested call to a callable returning an object with chainable methods SHOULD be placed on the same line, using the regular coding style for chained method calls on the new indentation level; e.g.:
$value = db_select('users') ->fields('users', array('uid')) ->condition('uid', $account->uid, '<>') ->condition(db_or() ->condition('mail', db_like($form_state['values']['mail']), 'LIKE') ->condition('name', db_like($form_state['values']['mail']), 'LIKE') ) ->range(0, 1) ->execute() ->fetchField();
- Opening and closing parentheses MUST use the same level of indentation across multiple lines.
Alternate proposal as at #69
As defined above but all multi-line structures, whether nested or not, must have each parameter/element each starting a new, indented line.
It is worth noting that this is essentially equivalent to what PSR and PEAR say about multi-line function calls and what PEAR
says about multi-line if
statements.
Original examples:
$foo = foo_function(
$term_1[$lancode][$delta]['value']['tid'],
$term_2[$lancode][$delta]['value']['tid'],
t('Translate some message here; good times.'),
t('Some other message here.'), array(
'foo' => 'thing',
'bar' => 'thing2',
),
$bar
);
$this->assertEqual(
$article_built[$field_name]['#items'][0]['fid'],
$default_images['field_new']->fid,
format_plural($count,
'An existing article node without an image has the expected default image file ID of @fid.',
'@count existing article nodes without an image have the expected default image file ID of @fid.', array(
'@fid' => $default_images['field_new']->fid
)
)
);
$result = format_string('@foo at some point before had the value of @bar', array(
'@foo' => $foo,
'@bar' => t('The Bar in @foo', array(
'@foo' => $foo,
)),
));
if (!isset($file)
// If no core compatibility version is defined at all, then it is
// impossible to check for updates.
|| !isset($file->info['core'])
// If the extension is not compatible with the current core, it cannot
// be enabled in the first place.
|| $file->info['core'] != DRUPAL_CORE_COMPATIBILITY
// If the new version requires a higher PHP version than the available,
// then it is not possible to update to it. The PHP version property
// defaults to DRUPAL_MINIMUM_PHP for all extensions.
|| version_compare(phpversion(), $file->info['php']) < 0) {
return TRUE;
}
Alternative examples:
$foo = foo_function(
$term_1[$lancode][$delta]['value']['tid'],
$term_2[$lancode][$delta]['value']['tid'],
t('Translate some message here; good times.'),
t('Some other message here.'),
array(
'foo' => 'thing',
'bar' => 'thing2',
),
$bar
);
$this->assertEqual(
$article_built[$field_name]['#items'][0]['fid'],
$default_images['field_new']->fid,
format_plural(
$count,
'An existing article node without an image has the expected default image file ID of @fid.',
'@count existing article nodes without an image have the expected default image file ID of @fid.',
array(
'@fid' => $default_images['field_new']->fid
)
)
);
$result = format_string(
'@foo at some point before had the value of @bar',
array(
'@foo' => $foo,
'@bar' => t(
'The Bar in @foo',
array(
'@foo' => $foo,
)
),
)
);
<?php
if (
!isset($file)
// If no core compatibility version is defined at all, then it is
// impossible to check for updates.
|| !isset($file->info['core'])
// If the extension is not compatible with the current core, it cannot
// be enabled in the first place.
|| $file->info['core'] != DRUPAL_CORE_COMPATIBILITY
// If the new version requires a higher PHP version than the available,
// then it is not possible to update to it. The PHP version property
// defaults to DRUPAL_MINIMUM_PHP for all extensions.
|| version_compare(phpversion(), $file->info['php']) < 0
) {
return TRUE;
}
?>
Remaining tasks
This issue is being divided into new small issues, See #95
- (done) Get more feedback from core developers.
- (done) Come to a consensus and define the policy.
- Document the new policy at http://drupal.org/coding-standards.
- Update Drupal Code Sniffer accordingly.
Original report by TravisCarden
Do we have a policy against breaking a function declaration across multiple lines? I'm wondering because Drupal Code Sniffer complains about array declarations inside function declarations that span longer than 80 characters, but I don't think anybody wants to break those into multiple lines. (For example, for visual folks, I don't think anybody wants to see anything like the below code.) I propose defining a policy, if necessary, and documenting it at http://drupal.org/coding-standards#functdecl. Assuming the policy is to not break up function declarations, I'll put an issue in to the sniffer as I can point to documentation on it. Thanks!
function accept_numbers($numbers = array(
'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine', 'ten',
)) {
// Function body.
}
Comment | File | Size | Author |
---|
Comments
Comment #1
TravisCarden CreditAttribution: TravisCarden commentedComment #2
xjmExpanding scope to reflect the current discussions in the cleanup issues.
Comment #3
jhodgdonMy personal view is that we shouldn't break function *declarations*.
But I'm not sure about the rest... What exactly do you mean by "language constructs"? I think we already have some policies on the coding standards page about breaking up if() statements, right?
Or is this just about arrays within these statements?
Comment #4
xjmI'd say about multiline formats in general, including arrays within other calls in particular. What we have so far about when not to wrap is only:
There is no comment about wrapping with regard to function declarations, function calls, flow control constructs like
while
orforeach
, etc.I'd propose:
if
,while
, andforeach
should never be wrapped onto multiple lines. E.g., do this:Never this:
If there is only one array parameter in a function call, then the function and array may be nested at the same level:
Comment #5
xjmI meant to add, outstanding question I have is if/when/whether closing parentheses should be on their own line. I'd lean toward yes since that's our policy with curlies, but I can also see a case for condensing it when there's no confusion (e.g. the last several parens when there are no subsequent lines before the semicolon).
Comment #6
jhodgdonI like #4. It is sensible and readable.
RE #5, I think all closing parens can be together on the last line, but they should be on their own line rather than put after the last item on the previous line.
Comment #7
sphism CreditAttribution: sphism commentedAny decision on things like this:
[function file_copy in file.inc]
should it be:
[edit]
Or should it be:
Comment #8
jhodgdonRE #7, I would vote for the last alternative of those three, I think, although if the first couple of parameters before the array were small, I might tend to prefer alternative 2.
Comment #9
TravisCarden CreditAttribution: TravisCarden commentedI like the third option in #7, too. It has the most symmetry and consistency, and it's always clear what structure something belongs to because the first line is right beneath its declaration. e.g. the first line of an array is always right beneath the
array
keyword, making it easy to scan.In passing, I'm waiting for a decision on this to push forward a lot of the issues in #1518116: [meta] Make Core pass Coder Review.
Comment #10
jhodgdonRE #9 - to reach a final decision, we probably need some more comments by core developers...
Comment #11
jhodgdonActually, in order to get comments easier, the next thing to do would probably be to make an issue summary.
Comment #12
TravisCarden CreditAttribution: TravisCarden commentedRe #11: Done.
Comment #12.0
TravisCarden CreditAttribution: TravisCarden commentedUpdated issue summary.
Comment #13
jhodgdonI updated the issue summary so it contains the actual proposal from xjm.
And regarding comment #7, I realized this case is covered by the proposed standards -- the last choice in comment #7 would be consistent with the standard, and in the summary, that would be item #4 "Nested arrays or function calls ...".
Anyway, I'm going to ask for feedback in IRC, and let's see if we can reach a consensus.
Comment #14
dww+1 for the proposal in the summary and comment #4. Looks clean, sane, readable, and consistent to me.
Comment #14.0
dwwAdd standards proposal to summary
Comment #15
dwwp.s. I just edited the proposal slightly to read:
It used to say "... by one space" but that didn't match the examples, nor the rest of our coding standards. I hope that's a "friendly amendment" to the proposal. ;)
Cheers,
-Derek
Comment #16
jhodgdonRE #15 - good catch, yes 2 spaces would definitely be the standard. :)
Comment #17
sphism CreditAttribution: sphism commentedYep, #4 looks good to me. I definitely prefer the closing )'s to be on separate lines indented to the same level as the line with the opening (
As opposed to the closing being eg )));
It seems that phpcs only cares if an array within the line exceeds 80 chars not if the overall line length exceeds 80 chars. Is that correct? What if one line contains 2 arrays of 41 chars each?
Comment #18
TravisCarden CreditAttribution: TravisCarden commentedThat is correct, @sphism; phpcs only cares if a line exceeding 80 characters contains a non-empty array definition. In the case you specify it will still complain because it measures the length of the line, not the length of the array definitions.
Comment #19
jhodgdonWe need to get this resolved -- there are a number of patches that are being held up pending this decision.
I haven't seen any "no" votes to the policy in the issue summary, so I am going to mark this tentatively RTBC, and if no one objects in a few days, I'll add it to the coding standards and we'll call it fixed. Sorry, we should have done that a while back -- this fell off my radar...
Comment #20
sun3 and 4 contradict our common coding practice throughout Drupal core; in particular these:
Our actual coding practice and semi-adopted standard is this instead:
Which, in turn, leads to an adjustment of those more complex/nested examples:
The latter does not have to be wrapped into multiple lines. But if you choose so, then use the style of the former to write it.
I guess what this bascially hints at is a standard for "options" function parameters, which has been non-obvious thus far.
It especially becomes relevant when taking additional parameters after the options parameter into account. Typical example:
This is the coding style that we're using almost exclusively everywhere throughout Drupal core. And so I think it only makes sense to formalize it exactly like that.
It probably becomes relevant to note this, because other PHP projects like Symfony use a completely different coding style for such things, which in turn means that developers can easily get confused when looking at the "wrong" files in the search for code examples.
Comment #21
sunFurthermore, on 2):
It does not make sense to me to disallow the multi-line syntax for control structures. There are perfectly legitimate use-cases for that; in particular in case of complex code conditions.
Whenever I see such in patch reviews, I usually demand for inline comments that explain what is being checked and why. In turn, that provides some reasoning for allowing the multi-line syntax.
For example:
Alas, this code actually slipped into core without any explanation at all. If I had reviewed or authored it, it would probably look like this today:
Note that these comments are made up; they may not be fully correct; didn't spend the time to check what this code actually does -- which in turn is the exact reason for why I encourage such inline comments.
Of course, you could say that "this can be written differently" to avoid the multi-line condition in the first place:
But as you can see, that leads to code duplication -- which may not matter in this case of simple Boolean return values, but can totally matter in other situations.
Of course, you can say that "even for those other situations this can be written differently" to avoid the code duplication:
However:
It is my strong belief that coding standards only exist to standardize how code should be written, if it gets written in a certain way. Coding standards should have no business in telling me which code logic I should use. The only exception to that are platform-specific or technical/performance related rules (which are rare in the first place).
In other words: If it makes more sense for my code to use a multi-line syntax, then I should be free to do so. The coding standards should be there to tell me how to write the multi-line syntax in that case. (They are not here to tell me that I'm not allowed to write my code that way.)
Comment #22
jhodgdonComment #21 -- I agree completely.
Comment #20 -- I am not sure I agree. In particular, this example:
This makes that first line extremely long... it seems like if you're going to break it up, it would be better to do as the issue proposal currently stands, which would be:
Or maybe I misunderstood -- were you advocating this as a good example of how to write code, or a bad example?
Maybe you can rewrite the proposed standards so it is clear what you are advocating to use as a standard?
Comment #23
dwwFWIW, I also disagree with the defacto "standard" in #20. I think that's much less readable than the new proposal. I'd rather adopt a better standard and then have to clean up some places than adopt a more clumsy standard just because that's what we mostly sorta do now.
Comment #24
sunIf you want to wrap, then you're free to do so, but without the unnecessary verbosity for the
array(
declaration; i.e.:I.e., only affecting the "options-style array" parameter for function calls (of which we have many and will likely have many more in the future).
At least from my perspective and experience, I can say that we're using this style after trying others, because it is the right balance between clarity and readability and verbosity and indentation-level-nightmare.
I will never ever write any code that uses an inane level of verbosity and indentations like this [copied from the summary] (even if you'd put it into the standards):
That being said, I have a not so ungrounded fear that people are going to run over all kind of core patches to set them to needs work, just because some functional code lines are exceeding 80 chars.
Our coding standards explicitly state that the 80 chars rule only applies to code comments, so this must not happen under any circumstances, and we better carefully clarify that these rules only apply if you write code that wraps.
And now that I check the actual proposed wording in the summary, it actually attempts to do exactly that :( -- 3. "recommends" wrapping function arguments when a line exceeds 80 chars.
So, next to aforementioned argument, this is also wrong, because the actual reason for recommending to wrap anything is to make the code more readable and understandable — which can be happily omitted, if the parameters passed to another function are "common sense" or otherwise clear already.
Furthermore, a limit of 80 chars for functional code is, in terms of real and actual Drupal code, way too narrow. You need to consider that function names like
drupal_array_unset_nested_value()
already consume 50% of 80 chars and that's only the bare and plain function name. And if that doesn't convince you, then just have a look at these 88 chars:So if we really need or want to place a number there, then at least based on my experience, 110 chars are much more realistic and compatible with Drupal.
However, even with 110, it does not always make sense to wrap a line of functional code, because when optimizing the readability of functional code, you should focus on the readability of its logic, so other programmers can understand it quicker. And yes, I fully acknowledge that this requires human sense and evaluation (which ultimately is one of the main points that make the difference between senior and junior programmers). But whether a machine or code parser warns about not wrapping lines or not is totally and absolutely irrelevant.
Lastly, this will also trigger confusions about wrapping strings in arguments (which we already see in the code base today), so it should be clarified that string arguments should not be wrapped. (Not sure whether there are exceptions to that rule though.)
Comment #25
jhodgdonThis issue is holding up a major effort underway to bring all of core into Coder compliance for 8.x. Is it intractable or can we come to an agreement? sun, you seem to be the only one objecting to the last proposed standard, so can you propose a new one that you think we can all agree upon?
Comment #26
sphism CreditAttribution: sphism commentedAgree with #25 - I don't mind how the code needs to be formatted, we just need a decision, this issue has really knocked the wind out of the Coder review code tidy up.
Comment #27
dww@sun: we love you, but you don't get veto power over stuff like this. We don't operate by consensus. One lone objection isn't enough to halt all progress. I say we move forward with the proposal in the summary. It's consistent, easy to teach text editors and IDEs to do for you, and already has a bunch of support in this issue.
Onward,
-Derek
Comment #28
jhodgdonBTW, I did just find a typo in the proposed standard [summary, item 4, second example]:
missing a comma at the end of that line. I'll fix the summary.
Comment #28.0
jhodgdons/one space/two spaces/ to match the actual example code (and the rest of our coding standards)
Comment #29
RobLoachCompletely off topic from this discussion. Don't mean to bike-shed, but out of curiosity, I ran Drupal through the PHP Coding Standards Fixer. The result is actually surprisingly readable.
Main notes in the changes:
{
else
's are on one line since it's not a function declaration like #1The most important thing about a coding standard is that it's consistent. It all simply comes down to just syntax, and if that syntax is consistent, nothing else matters.
Comment #32
jhodgdonRob: yes, off-topic. I think if you want to suggest using a formatter, that should be a different issue entirely.
Comment #32.0
jhodgdonadd missing comma (syntax error)
Comment #33
sunI thought I already provided sufficient and extensive reasons for why the original proposal won't work out. But well, forcing me to do a full rundown on a revised proposal now subtracted two hours from other core work. It's in the issue summary.
I've made sure to use W3C/RFC/spec terminology to separate rules from suggestions, recommendations, and options (which we generally should do some more in our coding standards).
Comment #34
sunSorry, just realized that #33 could be interpreted wrongly - I meant that my time is currently much better spent with working on the CMI architecture, which blocks a large range of issues. It has been stated here that the lack of this standard here would "block" some issues, but I don't really understand why and how that is possible; we've been able to write code in a sensible way in the past, so I don't get why we're trying to rush this coding standards addition in so quick.
Also, since any new hard boundaries being defined here would affect everyone writing code for Drupal, some more people should weigh in.
Comment #35
dww@sun:
That said, I'm going to choose to not spend a ton of time arguing with you over the details of our differences of opinion on what makes the code look more pretty. That's all that's happening here -- there's nothing objectively quantifiable to help us pick a given standard for this. It's just subjective opinion, and extremely bright people can have differences of opinion over things like this. But, for the record, I'm still -1 to "#20 + #21" (if that's what we have to call it), and +1 to "#4".
Cheers,
-Derek
Comment #36
sunIf it helps to split that one out, then I'm fairly sure we can do so immediately. I don't think I've ever seen any Drupal code using multi-line function declarations in my life.
Comment #37
TravisCarden CreditAttribution: TravisCarden commentedA comment on the motivation for this issue and the impetus for pushing it forward: This was prompted by work in #1518116: [meta] Make Core pass Coder Review, because of Drupal Code Sniffer (drupalcs) errors. (See Original report.) There's an effort underway to get drupalcs running on the test bot (#1382240: Integration with PIFR), but we can't have it going in and telling people to use multi-line function declarations! (Not that it's that close to happening.) Just a little background. :)
Comment #38
TravisCarden CreditAttribution: TravisCarden commentedAny more thoughts on this? All the same issues are, of course, still waiting on it.
Comment #39
sphism CreditAttribution: sphism commentedBump... If a decision is made on this then we can get on with the coder review tomorrow at drupalcon :)
If not then could we just ignore this 1 formatting issue, complete all our patches and reopen any files for which this is blocking as separate issues?
matt
Comment #40
xjmMy main concern with the new proposal is that it's just way too much text and specific instructions. We want to define a best practice guideline with some simple rules (that happen to already be implemented in code sniffer), not need a flowchart for deciding when to wrap lines.
I also don't like the introduction of 110 characters. This doesn't appear anywhere else in our coding standards and seems arbitrary. "If over 80, split the array onto multiple lines for better scanability" is far more consistent with our current standards for arrays defined by themselves and for line length generally.
Comment #41
sphism CreditAttribution: sphism commentedI actually quite like the idea of fixing everything except this 1 formatting issue.. then reopen issues for any files that are affected by it.
That way even if this issue is never resolved we've still fixed 90% of the errors
Comment #42
YesCT CreditAttribution: YesCT commentedI wanted to find out who wrote this *really* nice issue summary, so I looked at the revisions tab, but the user and date are wrong because of #1217286: Posting a comment changes too many issue node revision properties which is postponed because it will be fixed when the d7 d.o migrate happens. The reason I wanted to know who/when wrote the summary was because I wanted to know what comments were made after the good summary, so which were pertaining to that. I'm guessing the last update to the summary was #1539712-33: [policy, no patch] Coding standards for breaking function calls and language constructs across lines (comment 33)
Posted by sun on June 19, 2012 at 8:15pm
The next step for this issue is to get more core developers to comment on the proposal in the summary. sphism and I will try and round up a few.
Comment #43
attiks CreditAttribution: attiks commentedMy 2c on #20 + #21, some small remarks
1/ Please don't add another boundary
5/ so a while with a complex condition has to be written on one line, code copied from 3. I agree on the rule in the case of for/foreach
7/ I prefer ... MAY be wrapped ... this is mostly commen sense to not write lines of > 200 characters, but forcing people to break on a certain count is (for me) too much demanding.
My 2c on #4:
all fine by me
Comment #44
dixon_I have read through the summary, but really, this is the kind of things we should not spend this much time on right now. Code freeze is coming closer the last thing we want is to require all core developers to start re-roll patches because we changed the coding standards.
So please, let's just stick to what we have right now and not change anything. Our current standard has worked great for a long time.
Let's acknowledge that some patches might look a bit different than others, our standard might leave some room for that in this particular case.
We are all people with slightly different opinions. But let's see it as a beautiful thing - how our code grows organically ;)
Comment #45
jhodgdonYeah, what really needs to be done is that the Drupal Code Sniffer needs to not come up with standards of its own and complain when people make perfectly reasonable choices... which is what triggered this issue in the first place.
Comment #46
YesCT CreditAttribution: YesCT commentedThere was a suggestion to tell the code sniffer that certain ... tests ... are ok to be failed, and then things can proceed.
Comment #47
Lars Toomre CreditAttribution: Lars Toomre commentedThis issue appears to be holding up using Coder to perform reviews of core modules such as #1512434: Make Aggregator module pass Coder Review. It looks like there was some consensus formed which was use human judgement while encountering cases like the examples in the issue summary.
Is this a correct summary so that we can move forward on the other postponed issues?
Comment #48
thedavidmeister CreditAttribution: thedavidmeister commentedHey, I agree with a lot of what Sun is saying in #24 about wrapping functions.
I really hate this, I think it's hard to both read and write:
I don't think I've ever seen an example of functions being spaced like that t() is. Nobody I've ever worked with, or any code I've referenced from documentation/blogs Drupal or otherwise does that. Where was this idea adopted from, exactly?
Compulsory wrapping of function calls over 80 characters would be even worse in contrib than core - Remember that every contrib module currently prefixes the name of its functions with the name of the module and some contrib modules have quite long names.
I have a lot of perfectly readable single line function calls that are over 80 characters long in my contrib code simply because of indentation + long function names and don't really want them ending up in sparsetown.
I hate the idea of banning multi-line if statements where each new line begins with an || or && - I agree 100% with #21 and think that the example there isn't even a worst-case example of how complex if statements can get.
I love the idea of forcing inline comments for multi-line if statements.
Also, why is it ok to say we have to wrap function calls over 80 characters but we're no longer allowed to wrap if statements? what is it about if statements that makes them easier to read than function calls if they're on a single line?
For me I'm giving a bit +1 to "Proposed resolution 20 + 21" over "Proposed resolution 4".
The one thing I'd really like to remove from #20+21 is the arbitrary secondary character limit of 110, same as what was said in #43. Even Sun admitted that it's arbitrary and won't work 100% of the time.
Comment #49
sunI'd replace "Drupal" with "any PHP", and couldn't agree more. I've seen a lot of PHP code, but the proposed formatting here is beyond me.
Various patches recently slipped into core that are using this new style and all affected code is hardly readable. The extreme verboseness makes it really hard to decipher what the actual stack of calls and arguments is. You need to mentally remove all whitespace first, before you can make sense of the actual PHP code.
Comment #50
YesCT CreditAttribution: YesCT commentedOK.
Lets get a proposal here, that re-writes the standard/recommendation with "MAY" wrap, with an example of not wrapping, and an example of a reasonable wrap.
And, let's implement the check for *may* in the code sniffer, so we can get automation for most of the things there is consensus for.
I dont think that needs to be blocked on this.
Comment #51
thedavidmeister CreditAttribution: thedavidmeister commented@YesCT, isn't proposed resolution 20+21 /6 exactly those two examples? or are you looking for something more?
Comment #52
YesCT CreditAttribution: YesCT commentedThose are a good basis. But those need re-written in a more proposal/conclusion manner than a discussion one, and take into account some good suggestion that came after that too.
Could someone
1) read this really carefully
2) check the issue summary and update it to make sure the proposed resolution matches it (mostly 20/21, with the *may* and other good followup points)
3) post a comment to say that is done
4) someone else mark it RTBC
I get a little fuzzy with the next steps after that, maybe:
5) ... a core committer to say OK, and then mark it ... needs work?
6) update the standards doc ... http://drupal.org/coding-standards ... wait, not everyone can edit that page. so maybe if we get to this point, it needs to get assigned to someone who can edit that page
7) mark this issue fixed?
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commentedSo you want somebody to add a third proposed resolution that combines the two current ones + most recent criticisms?
Comment #54
YesCT CreditAttribution: YesCT commentedIt seems to me like that is needed. But if you dont think so. I'm happy to have someone skip to number 3 and 4: say that it's done and mark it RTBC.
I just want to unblock this and move it forward.
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commentedI'll have a shot now.
Comment #55.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #55.1
thedavidmeister CreditAttribution: thedavidmeister commentedAdded proposed revision as at #55
Comment #56
thedavidmeister CreditAttribution: thedavidmeister commentedI added a "proposal as at #55". It was largely based on 20+21 simply because I thought the wording was better with the MAY, SHOULD, RECOMMENDED, MUST style syntax.
Main differences between what I posted and 20+21 are:
1/ Removed specific reference to 110 characters in response to #40, #43, #48
2/ No changes
3/ Explicitly stated that inline comments need to be provided for each line of a multiline `if` statements in response to #21 and #48
4/ No changes
5/ No changes
6/ Removed reference to 110 characters in response to #40, #43, #48. Changed it to "when the length of individual paramters is very long".
Re-instated "especially when the function call includes other function calls or arrays." from proposal #4.
Added an array to the example.
Added further qualification to the example "Ie. Parameters that contain natural language strings, references to values deeply nested within arrays,
inline/"options-style" definitions of arrays or other parameters over 30 characters long are often good candidates to be considered "very long" parameters."
Added "Function calls should either wrap all parameters or none" with an example in response to #22.
7/ Removed reference to 110 characters in response to #40, #43, #48. Replaced with "array that contains more than one value" in response to #20.
8/ Removed reference to 110 characters in response to #40, #43, #48
9/ No changes
I also added "10/ Opening and closing parentheses MUST use the same level of indentation across multiple lines." in response to #17.
That should cover all the constructive criticism in this thread so far. Let me know if I missed anything.
Comment #57
thedavidmeister CreditAttribution: thedavidmeister commentedah, shouldn't be assigned to me any more.
Comment #57.0
thedavidmeister CreditAttribution: thedavidmeister commentedFix for previous revision
Comment #57.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary removed old proposals.
Comment #58
YesCT CreditAttribution: YesCT commentedsuper. I updated the issue summary to take out the older proposals so they dont confuse people. (see them by looking at the revision log if needed)
this is now really ready for someone to review and mark it RTBC if .... it's rtbc.
Comment #59
jhodgdonThanks for the new summary!! I have some definite concerns about the proposal that is now in the issue summary.
#3:
a) The || operator is at the beginning of each line. Which could be fine, but I'd like to know whether that's a standard we're adopting (if so, it should be mentioned in the notes on #3), someone's personal preference, or what. I think it's a lot more common to keep the operators on the previous line in code I've seen over the years (not necessarily PHP/Drupal), although the benefit of having the operators at the beginning of the line is that they are more easily scannable.
b) #3 also kind of implies that you can't split a conditional up unless you plan to document each condition with an in-line comment. Is that the case? Seems a bit restrictive.
#5:
I don't see why the rules for a while() should be different for an if(), since both use logical expressions.
#6:
I don't like the proposed example of how to wrap. In one of the lines, it has:
where t() is one of the function params, and array() is the next one. How come those are on the same line? The text says either to wrap all or none of the params in a function call, and this example has two params (or parts of two params) on the same line. I don't get how that can be a good style.
Overall:
Do we really need 10 belabored points added to our coding standards just for this? Truly, I'm inclined to just say "won't fix", remove the obviously bad standard that I think is still disgracing our standards page currently on this subject, and call it good. Or adopt a simpler standard, something like:
If you want to break long lines into multiple lines, be consistent in how you do it:
- If you are breaking an array declaration, put each element on its own line, rather than having a few elements on each line. Similarly with other structures.
- Never break up function declarations.
- Never break up for, while, etc. lines.
Comment #60
thedavidmeister CreditAttribution: thedavidmeister commentedI don't want to say "won't fix" as this issue effects me pretty often while I'm refactoring things that I find in the wild for legibility. Let's just keep at it until we get something that works - likely that we can come up with an abridged version once we're clear on the specifics.
- I've never seen anyone put the || or && at the end of the line as it makes it too hard to scan, I wasn't referencing any standard but it never occurred to me that putting the logical operator at the end of the line was an option. I guess we just hang out in different circles >.< I could probably dig up some references if needed.
- Yes, I was thinking that you shouldn't split an if statement into multiple lines unless it has comments as an `if` statement that doesn't require comments probably doesn't need to be split onto multiple lines. The multiline split for conditions is more about keeping things clear despite complexity in the logic than just reducing line length. Happy to add the word SHOULD instead of the implied MUST if you feel that's too restrictive.
- Seems fair to me that `while` can follow the same rules as `if` but I'd like to hear if anyone else has objections to that.
- I think the idea with the array thing is that the "array(" part of it is just a keyword that's allowed to stay inline and the first actual key/value of that array is considered the first part of "the parameter". You can tell that it is part of the array and not a new argument for the original function by the indentation. This was raised in #24 by sun:
I personally don't mind either syntax Sun's is more compact, but jhodgdon's example in #22 (excluding the incorrect closing parentheses) seems more consistent with what we're doing elsewhere with arrays. Would like to see some more discussion on this point...
Comment #61
webchickIt always depresses me when we have these long, drawn out discussions sucking in tons of key people in the project about how to make up our own Drupal-specific coding standards.
We really should align instead with one of the PSR-X working groups/standards and work to improve their docs where necessary. See for example https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-codi...
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commented@webchick, using a standard that isn't Drupal-specific makes sense to me, even if just because it reduces the amount of work we have to do ourselves.
At the moment though there's quite a few differences already between http://drupal.org/coding-standards and PSR-X. Even "simple" things like declaring a function or writing a switch statement are different in how they handle whitespace and braces/parentheses between the two standards.
I'm concerned that even though PSR-X is great, it doesn't appear to cover all the use-cases we've raised here. Notably there's no mention of multi-line `if` statements that I could see. Not sure if that means you can't do them, or there's just no accepted syntax for them yet :/
Is there an open issue for dropping Drupal's current standards and moving to PSR-X across the board? If so, that sounds like something that should be linked to from here as basically all meta-discussion around code styling becomes moot once that goes through.
In the meantime we could pretty easily re-write the current proposal, deferring everything that PSR-X has an opinion on to PSR-X and leaving only the "gaps" open to discussion here. That way we'd still be "PSR compatible" to the best of our knowledge, if we ever did make the switch.
Comment #63
jhodgdonSo... Can I just take a couple of steps back and say... I think we established our standards in the Drupal project, a number of years ago, because the code was a mess, and we needed to say "Put the {} here" and "Make sure there are spaces around operators" and things like that so that the code would become more readable and more uniform.
For the most part, I think our code is pretty readable and uniform now, and I wonder if all of this stuff about wrapping lines isn't mostly an edge case that doesn't come up very often that can be left completely unstated. Are there terrible code examples in our current code base or being proposed in patches that warrant these standards being established? Or is the only problem we really need to address that for some unknown reason Coder is advocating splitting things up that shouldn't be split up?
As a note, our current standards on wrapping are at:
http://drupal.org/coding-standards#linelength
My alternate proposal would be that we replace this with:
-----
There is no particular standard for how long code lines can be (but keep in mind that comment lines cannot exceed 80 characters, and longer code lines are generally less readable that shorter lines).
If you have an extremely long line of code that you want to wrap to break it into multiple lines, follow these guidelines:
- If you are breaking an array declaration, put each element on its own line, rather than having a few elements on each line. Follow a similar practice with other structures (for instance, a conditional should be broken up into one condition per line rather than several per line).
- Indent an additional two spaces when you break code up. If there is logical nesting in the structure you are breaking up, indent an additional two spaces with each level of nesting.
- Never break up function declarations, for() lines or foreach() lines. Keep them on a single line.
- Consider whether defining local variables to break up complex expressions into parts may make the code more readable (self-documenting if you choose good variable names) than just breaking up a long line by wrapping.
-----
Comment #64
thedavidmeister CreditAttribution: thedavidmeister commentedThe current standards say "In general, all lines of code should not be longer than 80 chars." which is not really reasonable for function calls.
As well as just extending the current standards page, we may need to update some of what's already there.
@jhodgdon, Webchick makes a totally valid point in that we should look to existing standards first before going off on our own tangent. The alternative proposal in #63 *seems* good, but for the purpose of discussion it would be good to see code examples along with the description for each rule - if only because at the moment statements like "Follow a similar practice with other structures" could be interpreted as either the current proposal's #6 or your own objection to it, depending on who you are.
I'll have a go at mixing the current proposal with PSR and I'll update the summary, I think you'll like it more than the current proposal as uses the verbose syntax you've been advocating from the start. If we can get a consensus on that, *then* let's look at trying to write a shorter synopsis that can be merged into the existing standards.
Wrapping is not so edge-case that we should be flippant about coming to a consensus or dismissive of concerns people are raising. By design the "options-style-array" has become almost ubiquitous in D7 and above for interacting with the API - if you don't have a standard for wrapping function calls the only way to keep your API calls readable is to define the array as a variable and pass it in every time, which is tedious and arguably not even "a good thing to do". The problem is exacerbated when t() starts appearing in those arrays.
Currently, in the wild you're much less likely to see:
Than this:
But really, is the first example even better? You're setting two variables that are presumably only going to be used once and thrown away, just to keep your code legible. What we're all saying that we want is something like (using PSR-2 here) but our coding standards don't yet make it clear how to achieve this:
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commentedI'm going to try making a mix of PSR + what we have and add it to the summary. It's probably worth seeing what comes out of that.
Comment #66
sunPlease note that adopting PSR-1 and PSR-2 has been discussed and won't fixed a couple of times already, and isn't really up for discussion.
In general, see #1693672: PSR-2 and PSR-1 for code formatting. (though there are duplicate issues)
Further reading here:
http://www.technosophos.com/content/when-standard-bad-standards-body
http://engineeredweb.com/blog/practical-problems-psr-2/
Comment #67
star-szrAt a quick glance the PEAR standards have a write-up on splitting up long if statements:
http://pear.php.net/manual/en/standards.control.php#standards.control.sp...
And for splitting function calls onto separate lines:
http://pear.php.net/manual/en/standards.funcalls.php#standards.funcalls....
These are at least closer to our standards than PSR and have examples.
Comment #68
thedavidmeister CreditAttribution: thedavidmeister commented@sun, good to know. I'm still going to post a summary that shows how it would look (for comparison) as I've pretty much finished it already.
@Cottser, I'm going to steal that paragraph about if statements from PEAR and make a few minor revisions to the current proposal as per #59 and #60 ;)
Unfortunately, both PSR and the PEAR standards go for a 4 space indentation, so regardless of whatever other inconsistencies there are we can't just use their examples verbatim for ourselves :(
Neither PEAR or PSR allow for putting the first argument of a function call inline with the function call, or pushing the 'array(' into the line above. I'm going to retract what I said earlier in #48 and just accept that I've always been "doing it wrong" as far as a few published standards are concerned >.<
Comment #68.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary remaining tasks
Comment #69
thedavidmeister CreditAttribution: thedavidmeister commentedmmk, I've updated the summary.
For all proposals:
- Added PEAR reasoning behind pushing operators to the start of a line in a multi-line
if
statement- Moved
where
to theif/else
rules- Stated that multi-line if statements SHOULD be commented
New things introduced by PSR-2:
- Explicit "soft" line limit of 120 characters
- Multi-line function *declarations* are allowed
I think that, given that the discussion so far explicitly pulled out those two things that means we *don't* want to adopt PSR-2 rules as things stand.
I also added my *interpretation* of the "alternative" proposal put forward in #63 but I might have got this wrong due to the way it was worded. The only difference between that and the current proposal that I could see seems to be that all multi-line structures are "one indented item per line" and so implicitly "first argument on first line" is not allowed in any context.
I'm personally going to +1 the "alternative" proposal as it stands at the moment because even though it is more verbose, it seems to be more in line with other PHP standards adopted elsewhere and it is more consistent by treating all multi-line structures as the same thing.
Let me know if I've gotten anything wrong. Thanks.
Edit: I just realised that the alternative proposal would also apply to
if
statements. Updating the summary now...Comment #69.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdate as at #68
Comment #70
thedavidmeister CreditAttribution: thedavidmeister commentedfwiw, i would like to be able to keep the first condition in an if statement on the first line, like in the PEAR standard.
Comment #71
joachim CreditAttribution: joachim commentedI'm generally in favour of the proposal, but it doesn't say what to do with along if() condition that has nested conditions, eg:
Should nested conditions go on one line? If they need to be broken because themselves they are too long, should they get indented an extra level?
Comment #71.0
joachim CreditAttribution: joachim commentedUpdate as at #69
Comment #72
donquixote CreditAttribution: donquixote commentedHi,
nice to see this being discussed.
I realize we should look at PSR-2 and other projects, but there is some room for bikeshed still left. So here are my thoughts..
Point 3 of the issue summary:
Keeping the operators at the beginning of the line has two advantages: It is trivial to comment out a particular line during development while keeping syntactically correct code (except of course the first line). Further is the logic kept at the front where it's not forgotten. Scanning such conditions is very easy since they are aligned below each other.
Very much agree.
I even played with the following sometimes, but I think that's a bad idea :)
Here every condition is on a newline and can be commented out or moved up or down, and have its own comment. TRUE (or 1) is the neutral element for &&, FALSE is the neutral element for ||. But again, I don't think this will get any support.
I wonder why we have 4 spaces indent before each && operator? In the rest of Drupal it is just 2 indent. And I don't see anything mentioned about this in PSR-2.
If we do it like below, then the first condition is even aligned with the others.
The
) {
should be on a new line for my taste.@joachim (#71):
I sometimes do this, but undecided about the closing bracket:
P.S.: I want to mention I personally prefer the yoda condition, but the current unwritten policy is against that, so I kept that out of the above examples.
Comment #73
jhodgdonCoding standards decisions are now supposed to be made by the TWG
Comment #74
mgiffordComment #75
tizzo CreditAttribution: tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Marking that it needs announcement for final discussion because it seems like we were close to a consensus before this stalled.
Comment #76
davidwbarratt CreditAttribution: davidwbarratt for Sail Venice commentedI would really like to be able to do something like this (which I think is the easiest to read):
The other ways that have been proposed would make this more difficult to read.
Comment #77
anoopjohn CreditAttribution: anoopjohn at Zyxware Technologies commentedWhat else needs to be done from outside of the Technology Working Group to take this forward?
Can we please also have an example of an if condition with multiple nested conditions. I believe it would be more readable when the operators at the same level are at the same indentation
Option 1
Option 2
Option 3
I prefer Option 3 as it makes the logic much more clearer than the other two. Also each condition is independent and conditions can be taken out or added at any level without causing inadvertent errors.
Comment #78
tim.plunkettOption 4
Comment #79
dawehnerAnother possible option:
Comment #80
tizzo CreditAttribution: tizzo commentedThe TWG discussed this on our call today - given that we have had new proposals since #75 that propose new options - we feel we need an update to the issue summary to finalize the options before announcing for final discussion. Tagging appropriately.
Comment #81
tizzo CreditAttribution: tizzo commentedUntagging needs announcement as we really need a summary update.
Comment #82
joachim CreditAttribution: joachim as a volunteer commented> Function declarations MUST be written on a single line; they should never wrap multiple lines.
The landscape has changed since that was written: constructors of classes with DI have horrendously long declarations, and they would be much easier to read and amend with wrapping.
Comment #83
joachim CreditAttribution: joachim as a volunteer commentedI think that means we've have this:
The declaration lines having the same indentation as function code puts me off a bit. I think it hinders readability because it makes it less clear where the declaration ends and the code begins. I'm not sure what we could do to improve that though.
I still think this is preferable to long constructors that go way off the screen in a text editor.
Comment #84
dwwIn places where I've seen function declarations split like this, I always thought the closing paren and opening curly brace are back at the same level of indentation as the "public function" keywords. Like so:
It's a small difference, but I think it helps the readability considerably, and (IMHO) completely solves the problem of not quickly seeing where the declaration ends and the code begins.
However, I'm not sure the best way to document the difference. Given this text you quoted:
there doesn't seem to be enough to specify which of these two indentation styles are intended. Perhaps something like this:
? Or something. ;)
And yes, I agree that either of these split-line approaches is preferable to doing it all on a huge single line.
Comment #85
joachim CreditAttribution: joachim as a volunteer commented> always thought the closing paren and opening curly brace are back at the same level of indentation as the "public function" keywords.
That looks much better!
Comment #86
dwwAlthough I don't want to risk starting a bikeshed war, if we do this for wrapping function declarations, I think it'd make sense to do a similar thing when wrapping
if ()
across multiple lines. For example, the issue summary has the following indentation/wrapping proposal for a longif ()
:I think the following is more readable, for similar reasons to why I think #84 is better:
In addition to being more readable (IMHO), it makes it easier to add more clauses without unnecessarily touching the last clause (reducing diffs and potential merge conflicts), makes it easy to comment out the last clause (which the motivation for this construct claims is possible, but actually isn't for both the first *and* last clauses), etc.
Comment #87
donquixote CreditAttribution: donquixote commentedYes, I very much support #84 and #86. I have done it like this for a long time, and it is the way to go.
In future PHP versions, PHP might even allow a comma after the last parameter. I think I saw this being discussed somewhere.
EDIT: See also #72
Comment #88
joachim CreditAttribution: joachim as a volunteer commented+1 to 86.
With regard to function declarations, I've just noticed how another benefit of the line wrapping is that it makes it really easy to see that the create() call and the __construct() parameters match up:
One line for each thing, and I can see how each call to the container for a service matches a parameter.
Comment #89
TravisCarden CreditAttribution: TravisCarden at Acquia commentedI'll add to @joachim's point that the benefit to readability of constructors and factory methods also applies to diffs that change them. On separate lines, you can read a diff involving multiple arguments with negligible mental effort. Put them all on one line and you have to shift gears and be careful not to overlook anything.
Comment #90
joachim CreditAttribution: joachim as a volunteer commentedThis has stalled for *checks watch* FIVE YEARS :(
Perhaps we should split it up into separate issues to make it easier to get to an agreement?
Comment #91
TravisCarden CreditAttribution: TravisCarden at Acquia commentedOh, it's no big deal, @joachim. I only opened the issue ten years ago. 😉
Are you thinking separate issues for each of function declarations and function calls, for example? It does seem that some opinions differed between the two, so separate issues makes sense to me, if you'd like to create some.
Comment #92
joachim CreditAttribution: joachim as a volunteer commented10 years! Eep :(
I'd say 3 issues:
- function calls
- function declarations
- language constructs (I can't see any examples of this in the issue so far, at least with a quick text search for the phrase)
Comment #93
jwilson3I've been stacking conditionals like this in custom glue code on Drupal projects for several years.
This is literally the last example in the issue summary called out as "Alternative examples", but this form has several benefits over the previous suggestions.
1.- the first conditional is on a line by itself so that all conditions line up with same indentation.
2.- #1 makes this consistent with multi-line functions args that start a newline after the opening paren, for coherence and consistency.
3.- Indent 2 characters not four so that IDEs configured for Drupal to indent by 2 will work automatically. The added benefit is that if indentation is consistent with multi-line function signatures and multi-line function calls that indent 2 chars, docs and examples are less confusing. 4-char indent appears to have been introduced when the closing part
) {
was at the end of the last conditional instead of on a line by itself, to differentiate between the code expressions inside the conditional versus the stacked logic conditions. #86 was pretty close except the indentation was left at 4 characters instead of changed to 2.4.- I would humbly propose placing the logical operators (
&&
and||
) at the end of the line instead of the beginning, to help readability and code elegance by having expressions line up vertically just like parameters in function signatures and function calls, a la:better than:
better than first conditional on one line:
better than 4 char indentation:
By "better" I basically mean easy to read, easy to write, consistent and maintainable.
Maintainability is important. if a patch adds a condition before or after any condition in the list, you see a very simple, readable one-line diff, as whitespace doesn't need to change as the code changes in the conditional...
better than:
better than:
Caveat 1: In these examples the conditionals have been shortened for brevity; in real-world these would typically be much longer lines of code like previous examples.
Caveat 2: I'm not a core developer as was called out in "Remaining Tasks" as being marked "done"... so I'm plainly aware my vote perhaps doesn't count as much as others here, but hopefully some of the points I bring up would resonate, and be considered before being labeled as bikeshedding.
There is some precedence for placing the logic operands at the end of the line.
https://github.com/prettier/plugin-php/blob/main/tests/if/if.php#L80-L94
Comment #94
quietone CreditAttribution: quietone at PreviousNext commentedThis issue was discussed at #3282227: Coding Standards Meeting 2022-06-07 2100 UTC.
We agree that progress will be easier if this were split into several issues. This was also suggested by @joachim in #90. And all the issues should use the language from the PSR coding standards.
What are the suggestions for how to split this up?
Comment #95
quietone CreditAttribution: quietone at PreviousNext commentedI read through the issue thinking about how to divide this up,
I agree with #90 that these 3 make sense, but I also think the line length discussion should be separate as well. I found two other issues about length. #3173782: Increase line length limit to 120 and #3039007: Relax the rule that arrays may not span more than 80 characters.
Comment #96
quietone CreditAttribution: quietone at PreviousNext commentedCreated a new issue for function declarations, #3295249: Allow multi-line function declarations
Comment #97
andypostAs PHP native attributes are used in core it's one more multiline statement with named arguments and sub-items