Comments

naveenvalecha created an issue. See original summary.

naveenvalecha’s picture

naveenvalecha’s picture

Issue tags: +Novice
miteshmap’s picture

Status: Active » Needs review
miteshmap’s picture

StatusFileSize
new6.34 KB

Status: Needs review » Needs work

The last submitted patch, 5: 2655016-4.patch, failed testing.

naveenvalecha’s picture

Issue tags: +Needs reroll

Thanks for the initial patch, its a great start.
I have comitted the #5 to a new coder branch for checking have we addressed all, and here's we need some more love http://pareview.sh/pareview/httpgitdrupalorgprojectsharethisgit-coder

Edit : Ignore the test failure.

miteshmap’s picture

Issue tags: -Needs reroll

@naveenvalecha: Thank you for the quick update.

I could apply the patch, so no reroll is needed.

miteshmap’s picture

Status: Needs work » Needs review
StatusFileSize
new3.36 KB
miteshmap’s picture

StatusFileSize
new27.07 KB

Added a new complete patch, previous patch was not good.

The last submitted patch, 9: 2655016-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2655016-10.patch, failed testing.

naveenvalecha’s picture

Thanks for the updated patch!! @miteshmap
I have committed the #10 to a coder branch for checking have we addressed all, and here's we need some more love http://pareview.sh/pareview/httpgitdrupalorgprojectsharethisgit-coder

We are just few steps away for pushing this in.

    Quick Overview :

  1. +++ b/src/Form/SharethisConfigurationForm.php
    @@ -347,7 +341,8 @@ class SharethisConfigurationForm extends ConfigFormBase {
    +    // remove anything that resembles code.
    

    why did we broke the comment line so early

  2. +++ b/src/Plugin/Block/SharethisBlock.php
    @@ -46,15 +46,15 @@ class SharethisBlock extends BlockBase implements ContainerFactoryPluginInterfac
    +   *   The module manager service.
    

    It would be "The sharethis Manager."

  3. +++ b/src/Plugin/Block/SharethisWidgetBlock.php
    @@ -49,15 +49,15 @@ class SharethisWidgetBlock extends BlockBase implements ContainerFactoryPluginIn
    +   *   The module manager service.
    

    It would be "The Sharethis Manager."

  4. +++ b/src/SharethisManagerInterface.php
    @@ -39,35 +39,40 @@ interface SharethisManagerInterface {
        */
    

    we also need a @return for the return type of the function

  5. +++ b/src/SharethisManagerInterface.php
    @@ -39,35 +39,40 @@ interface SharethisManagerInterface {
    +   * @param array $array
    +   * @param string $title
    +   * @param string $string
    

    we still need the description of these parameters.

  6. +++ b/src/Tests/SharethisBlockTest.php
    @@ -21,14 +21,20 @@ class SharethisBlockTest extends NodeTestBase {
    +  public static $modules = array(
    

    let's use the short arrays here and please don't broke the array elements line with starting element.

  7. +++ b/src/Tests/SharethisBlockTest.php
    @@ -21,14 +21,20 @@ class SharethisBlockTest extends NodeTestBase {
    +    $admin_user = $this->drupalCreateUser(array(
    

    Same here short arrays.

  8. +++ b/src/Tests/Views/SharethisViewsPluginTest.php
    @@ -32,7 +32,9 @@ class SharethisViewsPluginTest extends ViewTestBase {
    +  public static $modules = array(
    

    let's use the short arrays here and please don't broke the array elements line with starting element.

harsha012’s picture

StatusFileSize
new24.48 KB

Added patch as per Comment 13

naveenvalecha’s picture

+++ b/config/install/sharethis.settings.yml
@@ -1,7 +1,7 @@
+  comments: 0
+  late_load: 0
+  location: content

unrelated change.
I have no reviewed the patch b/c the the patch file size is also small but it should be more than #10

@harsha012,
please address the changes after applying #10, also provide the interdiff next time https://www.drupal.org/documentation/git/interdiff

snehi’s picture

Status: Needs work » Needs review
StatusFileSize
new352 bytes
new24.43 KB

Removing mentioned unrelated changes.

Status: Needs review » Needs work

The last submitted patch, 16: 2655016-16.patch, failed testing.

naveenvalecha’s picture

Title: Fix up the coding standards » Fix up the coding standards - Round 1
Status: Needs work » Fixed

I have committed the patch #10 , the rest of the patches were not correct.
Please don't post the patches without reading the thread.

naveenvalecha’s picture

I have created another followup issue for fixing the rest of the standards https://www.drupal.org/node/2655716
Thanks! for working on this.

Status: Fixed » Closed (fixed)

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