Skip to content
This repository was archived by the owner on Nov 21, 2019. It is now read-only.

Conversation

@thewilkybarkid
Copy link
Contributor

I can't see sniffs available anywhere that support multi-line method chaining (refs libero/schemas#6 (comment)).

This is an imperfect attempt to support it, without having to write dedicated sniffs.

phpcs.xml.dist Outdated
<rule ref="Libero"/>

<file>src/</file>
<file>Libero/</file>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the namespace being Libero\CodingStandard\Libero. (Is there a way to have PHP_CodeSniffer work without having the ruleset's parent directory being the name? It's already defined in the ruleset itself...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, think we'll have to revert this. The sniff name ends up being CodingStandard. WhiteSpace.ObjectStaticOperatorSpacing rather than the expected Libero. WhiteSpace.ObjectStaticOperatorSpacing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the decision here in the end? Happy either way

<?php

$foo->bar($arg1);
$foo->bar($arg1)->baz();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rather these became multiline.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is reasonable for me to allow an existing single-line statement to remain single-line, while if you want to go multi-line then you would have to follow a standard.

$foo->bar($arg1)->baz();

$foo
->bar(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rather not keep the newline.

->baz(
$arg1,
$arg2
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure about whether the semi-colon should go on the next line. Produces nicer diffs, but looks ugly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so this is enforced, even if you manually put it on the next line.

Copy link

@stephenwf stephenwf Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff might look something like this:

$foo
    ->bar(
        $arg1,
        $arg2
+    )
+    ->baz(
+        $arg1,
+        $arg2
     );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought semicolon-on-next-line was a Symfony standard by now

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd favor it not because of the diffs but because keeps lines independent (for fluent interfaces, which is the use case for chained calls):

  • add a new call
  • remove a call
  • change the order of calls
  • swap two calls
    become operations that only involve full lines

@thewilkybarkid thewilkybarkid added this to the 0.2.0 milestone Sep 24, 2018
@thewilkybarkid thewilkybarkid added the feature New feature or request label Sep 24, 2018
@thewilkybarkid thewilkybarkid requested a review from a team as a code owner October 1, 2018 09:08
@thewilkybarkid
Copy link
Contributor Author

@giorgiosironi Happy to merge? I'd like to update this to put the semi-colon on the next line, but will need a custom sniff I believe so will do it later.

Copy link

@giorgiosironi giorgiosironi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my latest comments, all reasonable

@thewilkybarkid thewilkybarkid merged commit 882fe25 into libero:master Oct 2, 2018
@thewilkybarkid thewilkybarkid deleted the method-chaining branch October 2, 2018 12:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants