Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heykarthikwithu created an issue. See original summary.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Active » Needs review
FileSize
1.07 KB

Added a patch for this.

joachim’s picture

Status: Needs review » Needs work

That's a good start, but could you possibly add a description to each of those too?

(And thanks -- I always like documentation patches! :)

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu
heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
1.24 KB
924 bytes

Added the description.

joachim’s picture

Status: Needs review » Needs work

Nearly there... :)

  1. +++ b/twitter_pull.module
    @@ -98,6 +98,13 @@ function twitter_pull_preprocess(&$variables, $hook) {
    + *     If you want to retweets to be included in the results. Defaults to TRUE.
    

    We typically don't write 'you' in docs. Typically, you'd write something like "Specifies whether retweets should be included in the results."

    And I should have noticed this earlier -- optional parameters say '(optional)' at the front of the description.

    Also this doesn't look like a boolean... NULL??? So we should say what the values ought to be.

    So:

    > (optional) TRUE to have retweets are included in the results; NULL to exclude them. Defaults to TRUE.

  2. +++ b/twitter_pull.module
    @@ -147,6 +154,9 @@ function twitter_pull_render($twitkey, $title = NULL, $num_items = NULL, $themek
    + *    Returns the tweets.
    

    You can just say 'An array of tweets' here. If you happen to be familiar with the array structure, it would be great to add that here -- eg, what are they keyed by, is each value in the array a string, or an array of data? -- but if not, it's ok like this. It's already an improvement :)

heykarthikwithu’s picture

Assigned: Unassigned » heykarthikwithu

working on this.

heykarthikwithu’s picture

Assigned: heykarthikwithu » Unassigned
Status: Needs work » Needs review
FileSize
1.27 KB
765 bytes

Added the changes.

heykarthikwithu’s picture

Priority: Normal » Minor
Issue tags: +Quick fix
joachim’s picture

Status: Needs review » Needs work

Thanks for the updated patch. Needs a bit more work though:

  1. +++ b/twitter_pull.module
    @@ -98,6 +98,13 @@ function twitter_pull_preprocess(&$variables, $hook) {
    + *     (optional) TRUE to have retweets are included in the results; NULL to exclude them. Defaults to TRUE.
    

    Doc lines need to wrap to 80 chars.

  2. +++ b/twitter_pull.module
    @@ -98,6 +98,13 @@ function twitter_pull_preprocess(&$variables, $hook) {
    + *     If you want to exclude replies
    

    This needs the same treatment as the other param: rewrite to remove 2nd person, '(optional)', specify the value if it's not a proper TRUE/FALSE, and the default.

rakesh.gectcr’s picture

FileSize
1.44 KB
1012 bytes
rakesh.gectcr’s picture

Status: Needs work » Needs review
joachim’s picture

+ *     (optional) TRUE to exclude replies; FALSE to don not exclude.
+ *     Defaults to NULL.

That sounds very weird!!!

Is that really what it does? If so, that's a whole other bug. The default value should be one of the values you're expected to pass!