Add missing '$' character to docblock for install_profile_info() function. [#1295500]

Attached patch fixes what I am sure is a simple typographical error:

--- a/includes/install.inc
+++ b/includes/install.inc
@@ -1183,9 +1183,9 @@ function drupal_check_module($module) {
  *    dependencies[] = dblog
  * @endcode
  *
- * @param profile
+ * @param $profile
  *   Name of profile.
- * @param locale
+ * @param $locale
  *   Name of locale used (if any).
  * @return
  *   The info array.

Comment	File	Size	Author
#11	install_profile_info-fix_docblock-1295500-11.patch	430 bytes	pillarsdotnet
#11
#9	install_profile_info-fix_missing_dollar_sign.patch	419 bytes	pillarsdotnet
#9
#5	includes--install_inc-fix_misc_doc_errors-1295500-5.patch	4.42 KB	pillarsdotnet
#5
#4	includes--install_inc-fix_missing_dollar_sign-and-blank_lines-and-indentation.patch	4.29 KB	pillarsdotnet
#4
#2	includes--install_inc-fix_missing_dollar_sign-and-blank_lines.patch	3.08 KB	pillarsdotnet
#2
	install_profile_info-fix_missing_dollar_sign.patch	419 bytes	pillarsdotnet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Lars Toomre CreditAttribution: Lars Toomre commented 30 September 2011 at 05:48

While you are in there, could you add a blank line before the @return doc statements? Thanks.

File	Size
includes--install_inc-fix_missing_dollar_sign-and-blank_lines.patch	3.08 KB

Comment #3

Lars Toomre CreditAttribution: Lars Toomre commented 30 September 2011 at 06:07

While reviewing the larger resulting patch, I see that drupal_verify_install_file() and drupal_install_mkdir() docblocks also have some indentation issues. The text below @param and @return directives should be two spaces. Since this is a documentation only patch, these probably should be cleaned up as well. Thanks.

Comment #4

pillarsdotnet CreditAttribution: pillarsdotnet commented 30 September 2011 at 06:11

File	Size
includes--install_inc-fix_missing_dollar_sign-and-blank_lines-and-indentation.patch	4.29 KB

As requested...

Comment #5

pillarsdotnet CreditAttribution: pillarsdotnet commented 30 September 2011 at 06:20

File	Size
includes--install_inc-fix_misc_doc_errors-1295500-5.patch	4.42 KB

OMG I just noticed this horrible travesty:

  * @return
- *  TRUE/FALSE whether or not the directory was successfully created.
+ *   TRUE/FALSE whether or not the directory was successfully created.
  */
 function drupal_install_mkdir($file, $mask, $message = TRUE) {

Must...fight...scope...creep...ARRRRGGGGHHHHH!!!!!!

I give up; re-rolled.

Comment #6

Lars Toomre CreditAttribution: Lars Toomre commented 30 September 2011 at 07:00

Status:

Needs review

» Reviewed & tested by the community

Thank you @pillarsdotnet. All of these changes look good to go and bring these docblocks closer to drupal documentation standards.

Edit: Assuming testbots return all green...

Comment #7

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 September 2011 at 22:05

Status:

Reviewed & tested by the community

» Needs work

Can you PLEASE limit your changes to the doc of the function mentioned in the issue statement? Cleaning up a whole file like this , AS YOU WELL KNOW pillarsdotnet, is not OK at this time.

Comment #8

Lars Toomre CreditAttribution: Lars Toomre commented 30 September 2011 at 22:15

I have no idea if this is cleaning up the whole underlying file. It is simply cleaning up the docblocks touched by the original issue and my suggestion to insert missing blank lines before @return doc statements.

What is wrong with getting this committed? Should not documentation-only patches correct noticed variations from documentation standards? I am at a loss as to why this now 'needs work'.

Comment #9

pillarsdotnet CreditAttribution: pillarsdotnet commented 30 September 2011 at 22:44

Status:

Needs work

» Needs review

File	Size
install_profile_info-fix_missing_dollar_sign.patch	419 bytes

Whatever. Re-uploading the original patch. Y'all feel free to continue arguing amongst yourselves.

Comment #10

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 1 October 2011 at 00:13

Status:

Needs review

» Needs work

This issue covers *one* function. Please just fix up the one function, and make it comply with doc standards, not the whole file?

Comment #11

pillarsdotnet CreditAttribution: pillarsdotnet commented 1 October 2011 at 00:20

Status:

Needs work

» Needs review

File	Size
install_profile_info-fix_docblock-1295500-11.patch	430 bytes

Fine, but when I submit separate patches for each of the other functions, both you and webchick will complain. She's already complained about the fact that I submitted separate patches for separate files as I found similar problems in each.

I really wish y'all would reach a consensus on these things. And publish it somewhere so I can re-read it when I forget.

Comment #12

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 October 2011 at 15:28

Status:

Needs review

» Reviewed & tested by the community

Here is my take on your meta-question of what to patch when:

a) Patches that do small standards updates to a whole file or to most of a file would tend to make it so that other patches in the queue cannot be applied, if they are not committed. So they can only be committed when the committers are receptive to such patches, such as the first week in November when every D8 patch in the queue is going to break anyway because the core files are being moved into a 'core' directory.

b) Patches that fix the same real issue in about 10 functions are not as disruptive, and are easier to review and commit than 10 smaller patches fixing the same real issue in the 10 functions separately.

c) When you are fixing up a docblock for one function, any patches around that area in the file would already be disrupted. So if there is one doc problem in a function doc, it's not more disruptive to, at the same time, fix up the rest of the doc problems in that one function at the same time, and it also doesn't make the patch much more time consuming to review or commit. So I advocate fixing up each function to comply with our current doc standards whenever a doc problem is being fixed for that function.

Does that make any kind of sense to you? I think it is pretty much common sense (you can get webchick, Dries, or catch to chime in and see if it makes sense to them too)... And if you have a good idea of where to publish it, I would be happy to write it up more formally. I don't think it belongs in coding standards... any thoughts on where it should go?

Anyway, the current patch on #11 should be fine for d7/8. Thanks!

Comment #13

pillarsdotnet CreditAttribution: pillarsdotnet commented 5 October 2011 at 15:48

Status:

Fixed

» Reviewed & tested by the community

any thoughts on where it should go?

Either:

Drupal patches

Explain how to create/submit a patch that is not too disruptive.

Or:

Patch review

Explain how to judge whether a patch should be marked "needs work" because it should be broken into two or more smaller patches.

Or possibly as a sub-page of one or both titled:

Patch scope

How much should I change in a single patch?

Seriously, without a written standard somewhere, I'm going to keep overreacting every time I receive contradictory instructions from two different developers who both outrank me.

Comment #14

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 3 October 2011 at 17:27

Good idea. Further discussion should go on this issue (which I filed to make sure this proposed page gets created):
#1298194: Standard needed for what is a "disruptive" patch