Followup to #2729829: Coding Standard fixes - Round 1

See http://pareview.sh/pareview/httpsgitdrupalorgprojectsharethisgit-7x-2x

CommentFileSizeAuthor
#10 coding_standard_fixes-2766919-9.patch5.42 KBManjit.Singh
#7 coding_standard_fixes-2766919-4.patch5.16 KBManjit.Singh
#4 coding_standard_fixes-2766919-3.patch5.14 KBManjit.Singh
#2 coding_standard_fixes-2766919-2.patch52.54 KByogeshmpawar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Yogesh Pawar created an issue. See original summary.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location 🇮🇳 Pune(MH)
CreditAttribution: yogeshmpawar as a volunteer and at QED42 commented
FileSize
coding_standard_fixes-2766919-2.patch52.54 KB

Updated patch for the coding standard fixes

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Status: Needs review » Needs work

Thanks for the patch. Patch will not apply.
pLease do the git pull origin 7.x-2.x on you local and after resolving merge conflicts post the patch then

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Needs review
FileSize
coding_standard_fixes-2766919-3.patch5.14 KB

Hey, I think most of the coding standard have already fixed in 7.x-2.x. But few of them were pending. So i have updated the patch. Please review it.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Status: Needs review » Needs work

Thanks for the patch!

  1. +++ b/ShareThisForm.css
@@ -1,3 +1,8 @@
+/**

    +1 

  2. +++ b/sharethis.module
@@ -7,7 +7,6 @@
  * Displays help and module information.

    Remove this description line in comment as well. Its not needed.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented

Did not understand the changes that you have mentioned in ShareThisForm.css Can you please elaborate ?

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Needs review
FileSize
coding_standard_fixes-2766919-4.patch5.16 KB

In the meantime i have updated the patch.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Status: Needs review » Needs work

#6
That was +1 from my side as well. Nice addition.

one more thing
http://cgit.drupalcode.org/sharethis/tree/ShareThisForm.js#n116
The variable i is not defined before usage. so it will show warnings. As we are on this.So define this variable as well.
Rest looks good to me.

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Issue summary: View changes
Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
FileSize
coding_standard_fixes-2766919-9.patch5.42 KB

Nice catch :)

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Needs review
navneet0693’s picture

navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented
Status: Needs review » Reviewed & tested by the community

Patch is working fine, clear to go :)

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Priority: Normal » Minor
Status: Reviewed & tested by the community » Fixed

Thanks! Committed and pushed to 7.x-2.x Will be included in next release.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented

Thanks a lot Nav (een & neet) ;)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.