First of a lot of style patches.
Bare me, this is just a test if I can handle this ;)
fixed some style issues, reported on the http://qa.drupal.org/head-style page
some points I need more help with
1)
public function rewind() {
// Nothing to do: our DatabaseStatement can't be rewound.
}
This gives the next warning:
Use PHP's master function, not an alias. (List of PHP aliases)
I think this is a false positive, and that means I can't do anything about it
2)
Function summaries should be one line only. (Drupal Docs)
I fixed the style issues so that it will pass. But maybe these sentences aren't correct now.
Yes a summary can exceed 80 characters, but it need to be on one line,
you can add more info but you need to sepperate it with an empty line.
3)
global variables should start with a single underscore followed by the module and another underscore
function _db_check_install_needed() {
global $databases;
if (empty($databases) && !drupal_installation_attempted()) {
include_once DRUPAL_ROOT . '/includes/install.inc';
install_goto('install.php');
}
}
problem with the $databases var, dunno how to fix this
I'll follow up this patch and can make follow up patches if I missed something
Comment | File | Size | Author |
---|---|---|---|
#58 | drupal.database-formatting.58.patch | 16.48 KB | sun |
#55 | drupal.database-formatting.55.patch | 16.53 KB | sun |
#50 | database_inc_style_12.patch | 37.08 KB | aspilicious |
#46 | database_inc_style_11.patch | 37.06 KB | aspilicious |
#44 | database_inc_style_10.patch | 37.06 KB | aspilicious |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedDamnit to much info in header and no patch atached
Comment #2
jhodgdonLooks mostly good. A few points:
a)
Actually, we don't normally have either array or Array or anything else on the line with @return.
b)
The first line of any doxygen doc header must be 1 sentence of less than 80 characters, ending with a period. You need to rewrite it so it fits into 80 characters.
This one is also a problem:
Again, it has to be less than 80 characers.
c) This sort of thing appears in several places:
I personally would not recommend wrapping this line inside a @code block in a doc header. If the Coder module is complaining about wrapping within @code, maybe Coder needs fixing, and maybe the style guidelines need fixing? That should be up for discussion, but probably on a separate issue.
d)
That sentence starting with "Adding..." is not a sentence. Should start with "Adds" instead, and then it would be fine.
Comment #3
aspilicious CreditAttribution: aspilicious commentedcopy paste of what I said in irc so everybody can follow
[20:32] jghodgon
[20:32] you are wrong in several points
[20:33] "Yes a summary can exceed 80 characters, but it need to be on one line,"
[20:33] thats what drupal is saying
[20:33] first line can exceed 80 characters
[20:34] and "If a parameter or return value is an array, object, or interface, it is recommended to specify that data type in the @param or @return directive.:"
[20:34] ==> example: @param Array $options
[20:34] see the Array data type
[20:35] Found those rules due to this link you alrdy know: http://drupal.org/node/1354
So I think a is correct and second part of b also, don't know why I have to wrap the first part of b
Comment #4
jhodgdonOK...
(a) is OK then. Sorry, I didn't know that had been added to the standards document, because it was just something someone decided to add rather than discussing it on an issue as they should have.
(b) is still wrong. The standard is that the first line should be 80 characters or less if possible. The standards page states that if it cannot possibly go longer than 80 characters, it still needs to be on one line and still needs to end with a . and still needs to have a blank line after it.
http://drupal.org/node/1354
(c) is not addressed on http://drupal.org/node/1354
(d) still needs to be fixed.
Comment #5
jhodgdonAlso, on that same standards page, in the section on documenting functions and methods:
So yes, our standard is that it should be 80 characters or less.
Comment #6
aspilicious CreditAttribution: aspilicious commentedSrry for the delay, another try!
My english isn't perfect so I can use a review :)
Comment #7
jhodgdonMost of this is pretty good... A few questions/comments:
a) I am not sure why you indented all of these:
b) I think the blank lines between the explanation and the @code need to be removed in this section:
c) This should be wrapped at close to 80 characters. Or if you think it should start a new paragraph (I don't think so, but maybe), you need to leave a blank line:
Also, that last line should probably end in :
d)
This should have been left as "array" and not "Array".
e)
The first line must be 1 sentence of less than 80 characters. How about:
"Exception for when popTransaction() is called with no active transaction."?
Except that it should start with a verb... well, we haven't actually defined our doc standards for classes yet, so let's leave that question for later.
f)
Should start with verb in 3rd person: "Executes" not "Execute".
Same applies to the next few functions.
g)
This should probably be all one sentence, or at least one paragraph? That second line there is not even a sentence.
h)
Leave out "this" at the beginning. Function description should start with a verb.
Comment #8
aspilicious CreditAttribution: aspilicious commentedd) is correct explained that earlier in this topic
a), b), c), f), h) ok I'll fix that
Need help with g)
What about e) ?
- Wrap it like I did, or leave it as it was
Comment #9
jhodgdong suggestion:
Retrieve the name of the currently active database driver; for example, "mysql" or "pgsql".
e: Don't wrap. Cut down to 80 characters or less. My suggestion was:
Exception for when popTransaction() is called with no active transaction.
Comment #10
aspilicious CreditAttribution: aspilicious commentedI'll finish this patch tomorow!
thnx jhodgon
Comment #11
aspilicious CreditAttribution: aspilicious commentedRetrieve the name of the currently active database driver; for example, "mysql" or "pgsql".
is to long so I changed it to:
Retrieves the name of the currently active database driver.
Comment #12
aspilicious CreditAttribution: aspilicious commentedComment #13
jhodgdonThere are still a few things that need fixing here:
a)
I think the blank lines at top and bottom should still be removed. This is all one paragraph, and a blank line in doxygen makes a new paragraph.
b)
Is it SelectQueryInterface or SelectQuery?
c) This one is also wrong:
d)
Again, this should be a lower-case 'array', not upper-case 'Array'.
e) This is not something you introduced, but it sure doesn't look like _db_error_page() actually ends the page request? Wouldn't it need to call exit() or something to do that? Hmmm...
Actually, it looks like it used to do a bunch of stuff in D6 that it no longer does in D7:
http://api.drupal.org/api/function/_db_error_page/6
http://api.drupal.org/api/function/_db_error_page/7
So the doc should now just say for D7 that it sets up the title and theme for an error page. It doesn't end the page request, and doesn't print the errors.
How about:
Or better yet, just remove the function? I cannot see it being used anywhere in Drupal core (I grepped through the code for the string error_page, just in case someone was calling it by building up the function name from a string or something, and found nothing except this function...). Do we still need it?
Comment #14
jhodgdonComment #15
aspilicious CreditAttribution: aspilicious commentedD is correct!
EDIT: it WAS correct, they changed the doc style document... pfff
==> I fixed it like the doc style document says it has to be done
b) I chose SelectQuery (I'm not sure)
Here is the code:
c) I fixed it
e) I just deleted the function for this patch
Comment #16
aspilicious CreditAttribution: aspilicious commentedComment #17
Crell CreditAttribution: Crell commentedThe ->select() method will always return an object that conforms to the SelectQueryInterface interface. 99% of the time that is either SelectQuery or a subclass thereof (which has an identical interface). I can't actually think of a way you'd do otherwise without writing your own driver and doing all sorts of unnecessary things with the query builders.
I originally documented it to return SelectQuery in the prose because generally it does (I think at the time that docblock was written there were no select query subclasses yet), and that's where you'll go to find the implementation code. When we added return types, I went with SelectQueryInterface because that is, strictly speaking, the correct and code-enforced expectation.
I'm fine with changing the prose, although I think it reads a bit clunkier for it to say SelectQueryInterface.
Comment #18
jhodgdonThen how about being a bit more verbose in all of those @return statements?
It just seems wrong to have
It should probalby say more anyway.
Comment #19
Crell CreditAttribution: Crell commentedI'm OK with that. Maybe something like:
And then a corresponding sentence for each of the factory methods on the connection object.
Comment #20
jhodgdonThat sounds reasonable to me.
By the way, that last patch has a slip of the finger in it:
near the top.
Comment #21
aspilicious CreditAttribution: aspilicious commented"And then a corresponding sentence for each of the factory methods on the connection object."
Can you enter the full comment in here so I can patch it?
Comment #23
jhodgdonRE #21: I think that's actually a separate issue to add things to doc in other files, so I would leave it for now. If you want to look into it, you'd need to find the methods on the connection objects for the different databases that generate SelectQuery objects.
Comment #24
aspilicious CreditAttribution: aspilicious commentedMaybe this one is good...
Maybe...
Comment #25
aspilicious CreditAttribution: aspilicious commentedstatus -> Needs review
Comment #26
aspilicious CreditAttribution: aspilicious commentedThis patch
- removes "function _db_error_page($error = '') {" as it is unused in core
- places newlines between doc blocks for example:
- makes the @code style consistent in the entire file (I think there need to be more info about intending code in the code style doc)
- shortens summaries to 80 characters
- removes newline at end of docblock
- renames a return value and adds more comment to another
That's it I guess
Comment #27
Dries CreditAttribution: Dries commentedLast patch looks good, but doesn't seem to apply to me. Might need a quick reroll.
Comment #28
jhodgdon#24: database_inc_style_5.diff queued for re-testing.
Comment #29
aspilicious CreditAttribution: aspilicious commentedIndeed last part needed reroll, hopefully last one :)
Comment #30
jhodgdonA few things need to be fixed with this latest patch:
a)
We don't put a parameter name like $queries on a @return.
b)
In order to (hopefully, if the API project ever gets around to doing the Right Thing with classes and interfaces) make a link here, can you write something like this (putting in the correct interface or class name):
Instead, call WhateverTheConnectionClassIs::startTransaction().
c)
This is the doc for db_query_range().
It's actually pretty important that the range stuff make it into the one-line summary, as this is the entire point of the function...
How about:
Executes a query against the active database, restricted to a range.
d) Grammar:
Should be "saves" here, and if "temporary" will fit on the line, that should be used rather than "temp".
e) Needs more in the one-line description:
What does "maps a generic data type" mean? Where's it mapping from and to?
Comment #31
aspilicious CreditAttribution: aspilicious commenteda)
Of course, I'll delete it but can you add a section in the style document that is saying that?
It's prety standard but I was confused.
b)
I think I found the base class
So I'm gonna change ->startTransaction() into DatabaseConnection::startTransaction()
c) Your sentece is good, going to use it
d)
Whatabout this? I don't know if it's saying exactly the same thing...
/**
* Maps a generic data type and its data size to an engine-specific data type.
*/
Comment #32
jhodgdonRegarding (a), we use $variable for parameters, but not for @return... Return values are not $variables, are they? None of the examples on the doxygen standards page have a variable... So I am not sure what should be added to the style guide.
Regarding (d)...
Hmmm...
I really have no idea what the return value of that function is from either the current or the proposed function header doc (or in general). It would be nice if the function doc header would explain what the function returns.
It should probably have @see WhateverTheSchemaInterfaceClassIs::getFieldTypeMap() in there somewhere, too?
Comment #33
aspilicious CreditAttribution: aspilicious commenteda) then its just me being dumb for a second :) never mind then
d)
I agree it isn't clear.
So you think adding @see DatabaseSchema::getFieldTypeMap() will do the trick?
I am not entirly sure DatabaseSchema is the interface/class we are looking for.
So it would be nice if some people with better understanding of databse can confirm this.
Comment #34
Crell CreditAttribution: Crell commentedI'm really not sure what the standard should be for referring to a method of an object. @see ClassName::method() is the most common I've seen, but can also be confused with a static method call. I guess it works for now, but I just want to be on record saying "Bah!" :-)
As for getFieldTypeMap(), I apologize for not getting around to rewriting that whole mess. Let's try the following as a docblock for DatabaseSchema::getFieldTypeMap():
And actually, now that I look at the code, db_type_map() should not exist. That data is only for internal use, and internal methods should use internal methods, not wrapper functions. So let's do this:
1) Add the docblock above to the DatabaseSchema::getFieldTypeMap() method.
2) In DatabaseSchema_mysql::processField(), replace the call to db_type_map() with $this->getFieldTypeMap(). It's probably my fault that wasn't done long ago already.
3) In the docblock at the top of includes/database/schema.inc, replace the reference to db_type_map() with one to DatabaseSchema::getFieldTypeMap().
4) Remove db_type_map() entirely as it has no purpose.
Comment #35
aspilicious CreditAttribution: aspilicious commentedThnx crell and jhodgon for clearing this out!
Without you two this issue would be dead and forgotten.
This patch is evolving in more then just a clean up...
I also found small style problems with the other database files (specially @link vs @see problems), but thats for an other issue.
I did everything commented above + changed two @links to an @see. Cause I think they were used wrongly. Jhodgon correct me if I'm wrong.
I don't think this is going to be the final patch ;)
Comment #36
aspilicious CreditAttribution: aspilicious commentedComment #37
jhodgdonaspilicoius: Thanks for all the patching and repatching!
Crell: Looks like ClassName::methodName() is pretty standard for PHP -- http://us3.php.net/manual/en/pdo.query.php
Latest patch:
save -> saves.
Rename -> Renames
Comment #38
aspilicious CreditAttribution: aspilicious commentedI managed to make a 1000+ line patch....
I found a lot of third person verb issues
Comment #39
jhodgdonWow... Of course by editing those lines you have opened up more issues...
Needs to be "prepares and returns". This occurs in the next several hunks of the patch as well.
info -> information
temp -> temporary
What does 'that' refer to? Maybe should be this?
Needs an end of sentence.
Needs a comma before "or" to conform to our doc standards for English style/usage.
You didn't modify this function header, but it doesn't conform (should be 80 characters, and I'm not sure how to rewrite it for 80?), and verb not in 3rd person...
Comment #40
jhodgdonComment #41
aspilicious CreditAttribution: aspilicious commentedSounds awfull lol, someone willing to give it a try?
I fixed the other issues.
Get ready for number 9
Whooeps I'm reading the file again and I'm seeing even more issues...
Help me with the first sentence
and another one
Comment #42
Crell CreditAttribution: Crell commentedDon't worry about transaction-related docblocks for now. Those are getting rewritten anyway in another issue that is rewriting the transaction code. :-)
Does the 80 character limit on the first line actually affect processing, or is it just stylistic? I would much rather us allow 85 character first-line summaries than waste time trying to bend the grammar to fit within that arbitrary limit.
Comment #43
jhodgdonSuggestions for these three spots:
1.
Sets a session variable specifying the lag time for ignoring a slave server.
2.
Decreases the depth of transaction nesting.
This function first attempts to decrease the number of layers of transaction nesting by one. If there was no active transaction, the function throws an exception. If this was the last transaction layer, the function either rolls back or commits the transaction, depending on whether the transaction was marked for rollback or not.
3.
Returns the result set as an associative array keyed by the given field.
Hmmm... This is the original database.inc for this method:
I think that last sentence about "note that..." can be omitted, as I don't think it adds anything to the documentation.
Comment #44
aspilicious CreditAttribution: aspilicious commentedWe are getting there...
Comment #45
jhodgdonRE #42: I am not sure about that -- I think it did once, but I'm not sure about now. Good question for the API project.
RE the latest patch: Still has
return -> returns -- see #39 comments.
I stopped there and didn't look at the rest...
Comment #46
aspilicious CreditAttribution: aspilicious commentedOeeps... CTRL-F missed something apparantly
Comment #47
jhodgdonThis still isn't quite right:
This needs another word -- "the current" what? I guess it's the current record... But then the @return says it returns a single field from the "next" record. So which is it?
When this is fixed, I am good with RTBC for this patch (finally). It looks great. Thanks for all of your hard work!!!!
Comment #48
aspilicious CreditAttribution: aspilicious commentedDatabase people have to say what it has to be cause it's an interface header so I can't see the implementation.
And I guess I have to change a test, where can I find the test that is failing?
Comment #49
jhodgdonThe failing test is "site-wide contact form". I don't think so... Probably just a testing glitch.
Regarding the DatabaseStatementInterface::fetchField method, it looks like there's a DatabaseStatementBase class that defines it as:
I checked the doc for PDOStatement::fetchColumn http://us2.php.net/manual/en/pdostatement.fetchcolumn.php
So I think the doc header for this one should say:
Returns a single field from the next record of a result set.
Comment #50
aspilicious CreditAttribution: aspilicious commentedWe will see what (maybe) someone else say but I think this sounds good.
Comment #51
aspilicious CreditAttribution: aspilicious commentedComment #52
jhodgdonYay! aspilicious deserves 1000+ thanks for all of this careful attention to detail, rerolls, etc.
Comment #53
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #54
andypostTransactions docs changes going in #669794: Use savepoints for nested transactions
Comment #55
sunYay, nice patch :)
Unfortunately, a small hiccup here.
While being there, also fixed some other formatting issues.
Powered by Dreditor.
Comment #56
aspilicious CreditAttribution: aspilicious commentedLooks good
Comment #57
jhodgdonNot quite:
We never want doc lines to wrap past 80 characters, and this one does.
And
This doesn't follow our list standards, really.
Bad grammar/style.
And does the API module support @throws currently? If not, we need to file an issue on the API project.
Comment #58
sun1) Fixed. However, see #502190-9: Hook implementation headers out of compliance with standards
2) Not invented here. Note that this does not describe array keys. Further, you noticed that those lines belong to a method that is commented out?
3) Basically see 2).
4) AFAIK, @throws is not supported yet. But there's a complete rewrite of API module on the way, if I'm not mistaken.
Comment #59
Dries CreditAttribution: Dries commentedCommitted the patch in #58 but we can definitely work on follow-up patches for items (2) and (3). Marking as 'fixed' for now.