Skip to content
This repository has been archived by the owner on Dec 3, 2023. It is now read-only.

[CodingStandard] Add ClassNameSuffixByParentFixer #633

Merged
merged 18 commits into from Feb 8, 2018

Conversation

TomasVotruba
Copy link
Member

Closes #607

Copy link
Contributor

@carusogabriel carusogabriel left a comment

Choose a reason for hiding this comment

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

Nice to have this one 👏

$classTokensAnalyzer = ClassWrapper::createFromTokensArrayStartPosition($tokens, $index);
dump($classTokensAnalyzer->getName());
dump($classTokensAnalyzer->getParentClassName());
die;
Copy link
Member

Choose a reason for hiding this comment

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

this looks like debug code

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, sorry

'<?php
class SomeClass extends Command
{
}'
Copy link
Member

Choose a reason for hiding this comment

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

Personally I recommend using NEWDOC syntax for multiline strings.

public function configure(?array $configuration = null): void
{
if ($configuration === null) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this does what it's supposed to. Quoting from ConfigurableFixerInterface::configure():

     * New configuration must override current one, not patch it.
     * Using `null` makes fixer to use default configuration (or reset configuration from previously configured back
     * to default one).

Copy link
Member Author

Choose a reason for hiding this comment

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

How the code should look like then?

Copy link
Member

Choose a reason for hiding this comment

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

From what I see in default fixtures it should call $this->configuration = $this->getConfigurationDefinition()->resolve([]);. However it is not too important because

            @trigger_error(
                'Passing NULL to set default configuration is deprecated and will not be supported in 3.0, use an empty array instead.',
                E_USER_DEPRECATED
            );

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I already call that in ctor to prevent missed configuration call.

So here it doesn't have sense anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, I already call that in ctor to prevent missed configuration call.

So here it doesn't have sense anymore

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Feb 7, 2018

I've added support for interface types as well.

Any ideas to improve default type map?

    /**
     * @var string[]
     */
    private $defaultParentClassToSuffixMap = [
        '*Command' => 'Command',
        '*Controller' => 'Controller',
        '*Repository' => 'Repository',
        '*Presenter' => 'Presenter',
        '*Request' => 'Request',
        '*EventSubscriber' => 'EventSubscriber',
        '*FixerInterface' => 'Fixer',
        '*Sniff' => 'Sniff',
    ];

@enumag
Copy link
Member

enumag commented Feb 7, 2018

Maybe this should be added.

        '*Exception' => 'Exception',

@TomasVotruba
Copy link
Member Author

Copy link
Contributor

@JanMikes JanMikes left a comment

Choose a reason for hiding this comment

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

I like the idea! It will be useful in my projects as well.

@TomasVotruba
Copy link
Member Author

@lexinek Thanks bro! Any thing to add to the list? CommandHandler maybe?

@JanMikes
Copy link
Contributor

JanMikes commented Feb 7, 2018

Yeah i was thinking about it, but i think these are more specific and i think more individual. Could be Request, Handler, Response, Repository. For doctrine custom types the Type suffix. Test for PHPUnit testcases and so on 😄 Processor for Sentry processors, Producer and Consumer for rabbit.

There are really alot of them, are you sure you would like to add them all here instead of giving more freedom to users? Because naming conventions can be very different in different companies.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Feb 7, 2018

@lexinek Well, the fixer is customizable, so anyone can use anything they like 🏳️
Thank for the ideas, I missed most of them :). I'll think about it tomorrow and add those most common ones.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Feb 8, 2018

I liked Test, but is not deterministic, because there are 2 options:

  • TestCase => Test
  • TestCase => TestCase

@TomasVotruba
Copy link
Member Author

I've added support for interface version as well

So EventSubscriber covers also EventSubscriberInterface

@TomasVotruba TomasVotruba merged commit 33ab2d3 into master Feb 8, 2018
@TomasVotruba TomasVotruba deleted the cs-parent-name-rule branch February 8, 2018 14:58
@deprecated-packages deprecated-packages locked as resolved and limited conversation to collaborators Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[CodingStandard] checker idea - class suffix naming by parent class
4 participants