Enhancement: Enable no_alias_function and trailing_comma_in_multiline_array fixer by localheinz · Pull Request #491 · jsonrainbow/json-schema

localheinz · 2017-12-31T12:21:25Z

This PR

enables the no_alias_function as well as the trailing_comma_in_multiline_array fixer
runs composer style-fix

💁‍♂️ For reference, see https://github.com/FriendsOfPHP/PHP-CS-Fixer/tree/2.9#usage:

no_alias_functions [@Symfony:risky]

Master functions shall be used instead of aliases.

Risky rule: risky when any of the alias functions are overridden.

erayd · 2017-12-31T22:56:14Z

.php_cs.dist

        'yoda_style' => null,
-        'increment_style' => false,
    ))
+    ->setRiskyAllowed(true)


I find this concerning - there have been a number of times in the past when php-cs-fixer has shipped changes that resulted in it altering the execution logic of the project code, rather than making purely cosmetic changes. Thankfully, this caused the testsuite to fail, so nothing broken made it through review, but still. Anything that php-cs-fixer labels risky, or has the potential to change execution logic, worries me.

To clarify - php-cs-fixer has altered logic with no warning, and no changes in configuration. This behaviour is IMO unacceptable. I don't have an issue with the join -> implode changes it's made this time, but if we're going to commit this, I'd like some certainty it won't make changes in the future that break things (e.g. by replacing deprecated function names that are required for PHP 5.3 compatibility).

That's why I would suggest to pin friendsofphp/php-cs-fixer to at least a specific minor version, what do you think?

I think pinning is a very good idea. It doesn't entirely solve the issue, but it will help.

@mathroc - php-cs-fixer is yours, how do you feel about this?

erayd · 2017-12-31T22:59:10Z

.php_cs.dist

        'pre_increment' => false,
-        'trailing_comma_in_multiline_array' => false,
        'simplified_null_return' => false,
+        'trailing_comma_in_multiline_array' => false,


While we're moving it, can we please turn this rule on? A trailing comma is desirable, as it makes git blame more obvious.

Sure, can do!

localheinz · 2018-06-22T19:10:41Z

@bighappyface

Rebased! Apologies for the delay.

bighappyface · 2018-10-16T13:58:30Z

@mathroc please see the conversation on the initial commit of this PR - 4740600#r159148409

mathroc · 2018-10-16T19:46:08Z

@erayd @bighappyface I think there's been a misunderstanding, why do you think php-cs-fixer is mine ?

erayd · 2018-10-16T20:08:05Z

@mathroc @bighappyface This is my error sorry - looks like I tagged the wrong person. The person who was supposed to have been tagged was @alcohol.

localheinz · 2018-10-16T20:23:24Z

@erayd

Can I help in any way?

alcohol · 2018-10-16T21:07:13Z

Uhm, mine? What?

erayd · 2018-10-16T21:32:57Z

@alcohol Yours, because you added it to this project in the first place. I was wanting to know what your thoughts are on whether pinning it is an acceptable solution to the problem of php-cs-fixer making unexpected and unannounced changes to the execution logic, as I assume you had reasons for setting it up the way you did originally.

@localheinz Testing this PR against the latest version [of php-cs-fixer] would be helpful. It's been close to a year since this PR was initially opened, and I don't have time to do that at present (plus I'm not terribly familiar with php-cs-fixer anyway).

alcohol · 2018-10-17T06:31:31Z

Oh, I see.

Having read up on the discussion (including the hidden comments), I can add the following: pinning is fine.

Though it would be a good idea to occasionally check for updates using composer outdated --direct and determining what changes have been made and whether or not they warrant an update (and if yes, what would have to be configured additionally etc).

localheinz · 2018-10-17T07:14:40Z

@erayd

Just rebased and pushed, let me know what I can do!

alcohol · 2018-10-17T07:19:03Z

You did not actually pin php-cs-fixer to a minor version yet though?

I would suggest changing

"friendsofphp/php-cs-fixer": "^2.1",

to

"friendsofphp/php-cs-fixer": "~2.2.20",

localheinz · 2018-10-17T07:47:33Z

@alcohol

No, that was never my intention with this PR, but can do in a separate PR.

bighappyface · 2018-10-19T15:46:05Z

@alcohol @localheinz PHP-CS-Fixer has been pinned per #537

localheinz · 2018-10-19T17:21:50Z

@bighappyface

Rebased!

bighappyface · 2018-10-19T17:44:18Z

@localheinz please rebase once more. I merged #535.

localheinz · 2018-10-19T17:47:49Z

@bighappyface

Done! 🤓

bighappyface

+1 for 53a519e
+1 for ecf1ce6

bighappyface · 2018-10-19T18:06:55Z

@erayd @shmax any objections to merging?

localheinz · 2019-11-13T10:13:57Z

Closing in favour of sorting out other issues with the php-cs-fixer requirements and configuration first.

erayd requested changes Dec 31, 2017

View reviewed changes

localheinz force-pushed the feature/no-alias-function branch 2 times, most recently from ead8c8a to 531ff9f Compare January 1, 2018 00:55

localheinz changed the title ~~Enhancement: Enable no_alias_function fixer~~ Enhancement: Enable no_alias_function and trailing_comma_in_multiline_array fixer Jan 1, 2018

localheinz force-pushed the feature/no-alias-function branch 2 times, most recently from 3ad766b to c4c6bcd Compare January 1, 2018 17:24

bighappyface added the Needs rebase on target branch label May 23, 2018

localheinz force-pushed the feature/no-alias-function branch 2 times, most recently from 2cb0bef to 7662df2 Compare June 22, 2018 18:28

localheinz force-pushed the feature/no-alias-function branch from 7662df2 to 8de75c1 Compare October 17, 2018 07:07

localheinz mentioned this pull request Oct 17, 2018

Fix: Pin friendsofphp/php-cs-fixer to minor version #537

Merged

2 tasks

localheinz force-pushed the feature/no-alias-function branch 3 times, most recently from b908726 to 3171d9a Compare October 19, 2018 17:21

localheinz force-pushed the feature/no-alias-function branch from 6b0e068 to 53a519e Compare October 19, 2018 17:47

bighappyface removed the Needs rebase on target branch label Oct 19, 2018

bighappyface approved these changes Oct 19, 2018

View reviewed changes

localheinz force-pushed the feature/no-alias-function branch from ecf1ce6 to 3a565fa Compare June 18, 2019 05:53

Enhancement: Enable no_alias_functions fixer

7898741

localheinz force-pushed the feature/no-alias-function branch from 3a565fa to 7898741 Compare November 13, 2019 09:40

localheinz added 2 commits November 13, 2019 10:41

Enhancement: Allow risky fixers

914b4cc

Fix: Run 'composer style-fix'

011bf7b

localheinz closed this Nov 13, 2019

localheinz deleted the feature/no-alias-function branch November 13, 2019 10:13

Uh oh!

Conversation

localheinz commented Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erayd Dec 31, 2017

Choose a reason for hiding this comment

Uh oh!

erayd Dec 31, 2017

Choose a reason for hiding this comment

Uh oh!

localheinz Dec 31, 2017

Choose a reason for hiding this comment

Uh oh!

erayd Jan 1, 2018

Choose a reason for hiding this comment

Uh oh!

erayd Dec 31, 2017

Choose a reason for hiding this comment

Uh oh!

localheinz Dec 31, 2017

Choose a reason for hiding this comment

Uh oh!

localheinz commented Jun 22, 2018

Uh oh!

bighappyface commented Oct 16, 2018

Uh oh!

mathroc commented Oct 16, 2018

Uh oh!

erayd commented Oct 16, 2018

Uh oh!

localheinz commented Oct 16, 2018

Uh oh!

alcohol commented Oct 16, 2018

Uh oh!

erayd commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alcohol commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

localheinz commented Oct 17, 2018

Uh oh!

alcohol commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

localheinz commented Oct 17, 2018

Uh oh!

bighappyface commented Oct 19, 2018

Uh oh!

localheinz commented Oct 19, 2018

Uh oh!

bighappyface commented Oct 19, 2018

Uh oh!

localheinz commented Oct 19, 2018

Uh oh!

bighappyface left a comment

Choose a reason for hiding this comment

Uh oh!

bighappyface commented Oct 19, 2018

Uh oh!

localheinz commented Nov 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

localheinz commented Dec 31, 2017 •

edited

Loading

erayd commented Oct 16, 2018 •

edited

Loading

alcohol commented Oct 17, 2018 •

edited

Loading

alcohol commented Oct 17, 2018 •

edited

Loading