Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Here is a simple patch that adds consistency to schema indentation and increase code readability.
Comment | File | Size | Author |
---|---|---|---|
#11 | schema_identation.patch | 56.54 KB | recidive |
#10 | schema_identation.patch | 50.82 KB | recidive |
schema_identation.patch | 50.83 KB | recidive | |
Comments
Comment #1
ScoutBaker CreditAttribution: ScoutBaker commentedSome of the things I noted which appear to go directly against the coding standards at http://drupal.org/coding-standards:
You removed extra spaces on assignments that promoted readability.
Moving comments down to their own line could lead to confusion. If I didn't see the before-and-after versions in the patch, I would not know which line the comment applies to.
The closing parentheses and comma for an array definition should follow the final element of the array. You can reference the example shown for arrays in the coding standards (see link above).
I can't apply this right now to check it myself, but I'm also wondering what the Coder module would say with these changes applied since it is supposed to validate against the coding standards.
Comment #2
recidive CreditAttribution: recidive commentedThey are not currently consistent and personally, I think the extra spaces don't add too much readability to be considered. They tend to break consistency as new elements/assignments are added, and if a property with bigger length is added, more lines should be changed to conform, which means bigger patches. But these are just my personal thoughts.
Usually, in-line comments applies to the line bellow the comment.
Are you sure? See the example for the form element.
AFAIK, coder module doesn't check code indentation.
Comment #3
webchickOn a quick visual inspection, this looks great and is much more consistent with the rest of core. There were about 10 of us working on Schema API documentation, so I'm not shocked that there are a few inconsistencies (or, ok, more than a few ;)). I'll try to give it a more thorough review as time permits.
Comment #4
ScoutBaker CreditAttribution: ScoutBaker commented@recidive:
#1 - removing spaces - I'm simply pointing out that the change made directly contradicts what is stated as an acceptable coding standard. As a personal preference, rather than a strict coding standard violation, it would be just as acceptable to submit a patch that adds justifying spaces everywhere. To put this in perspective, your preference is to not have the extra spaces; in this case majority rules, and I'm neutral.
#2 - inline comments - You said it yourself, "usually." IMHO, it would be clearer if the comment wording is changed to facilitate the change. People often shortcut or word differently when they write comments that appear on the same line as the code, versus a separated comment. If everyone thinks this is fine as-is, OK. I
#3 - closing parens - Good point. I did look at that and for some reason I was looking at the t() function ending. Thanks for pointing it out. I'll get my eyes checked and try not to make the same mistake again. :)
#4 - coder module - There are a handful of indentation issues that it shows for core. I don't know if it specifically is checking the spacing/indentation with regards to array declarations. I suppose I wouldn't expect it to since one or more spaces is allowable.
Comment #5
webchick@ScountBaker: Regardless of what the coding standards document says, if there are any questions about issues of indentation, etc. etc. we go by "What Would Drupal Core Do?" (WWDCD ;))
Unfortunately, the coding style document is not 100% reliable since:
a) it has to be manually updated
b) anyone with a CVS account can do so
c) it's not updated often
d) when it is, the copy on the d.o website often falls out of sync with the copy in CVS, which is more 'definitive'
And in terms of WWDCD, recidive's patch is sound.
Comment #6
ScoutBaker CreditAttribution: ScoutBaker commented@webchick: Thank you for the information. It is unfortunate that such an important document appears to be in a state of "disrepair", especially as it is directly linked to in the Contributor links. I'll have to consider contributing to the documentation when I have more knowledge.
Comment #7
webchickThat would be awesome. :D I always get excited about new contributors with interest in coding standards!
FYI: The very latest version of the Coding Standards is always available at http://cvs.drupal.org/viewvc.py/drupal/contributions/CODING_STANDARDS.ht.... If you see things that are missing or unclear, please file a bug in the Documentation queue.
Comment #8
gpk CreditAttribution: gpk commented[slightly off-topic I know]
How about changing the http://drupal.org/coding-standards page so it gets its content directly from CVS (or the API docs, or somewhere) -> avoids maintaining 2 versions ...
Comment #9
webchickIt does, but it needs to be manually updated by someone with shell access, which doesn't happen very often. Discussion at http://drupal.org/node/204751 on how to improve this.
Comment #10
recidive CreditAttribution: recidive commentedRefreshing the patch.
Comment #11
recidive CreditAttribution: recidive commentedRefreshing the patch.
I've changed it to keep the extra spaces where they seems consistent.
Comment #12
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks all.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.