Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat] Add PHPCsFixer wrapper #219

Merged
merged 29 commits into from
Sep 23, 2019

Conversation

Jibbarth
Copy link
Collaborator

@Jibbarth Jibbarth commented Jul 13, 2019

Q A
Bug fix? no
New feature? yes
Fixed tickets #...

It's still not a "fix" option that will correct code direclty, but this allow to use Fixer from PhpCsFixer to indicate how should be some files. It wrap theses fixer into our "Insights" 馃槈

image

I have been mainly helped with this site to get usefull fixer to add in our metrics.

I tried to add only those that I did not found match in PhpCodeSniffer.

This PR will be complete when:

@Jibbarth
Copy link
Collaborator Author

Look at this build for a complete example

@olivernybroe
Copy link
Collaborator

This is awesome!
Amazing work on this. I'll try and look into the code :)

Copy link
Collaborator

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

I really like this!
Looking forward to it being part of the package. 馃帄

src/Application/Injectors/FileProcessors.php Outdated Show resolved Hide resolved
src/Domain/Differ.php Show resolved Hide resolved
src/Domain/FixerFileProcessor.php Outdated Show resolved Hide resolved
src/Domain/Insights/InsightFactory.php Outdated Show resolved Hide resolved
src/Domain/Insights/InsightFactory.php Outdated Show resolved Hide resolved
@nunomaduro
Copy link
Owner

This sounds very promising. 馃敟

@Jibbarth Jibbarth force-pushed the feature/php-cs-fixer-as-insights branch from a4c4b25 to 874fc69 Compare July 14, 2019 18:18
@Jibbarth Jibbarth added the enhancement New feature or request label Jul 14, 2019
@nunomaduro
Copy link
Owner

@Jibbarth This pull request is ready to validated buddy? 馃摝

@Jibbarth
Copy link
Collaborator Author

@nunomaduro Sorry buddy, busy week 馃槄 This PR is not yet ready, I have to add at least a way to handle configuration.

@Jibbarth Jibbarth force-pushed the feature/php-cs-fixer-as-insights branch 2 times, most recently from d5dd980 to 5a16835 Compare July 20, 2019 13:06
@Jibbarth Jibbarth force-pushed the feature/php-cs-fixer-as-insights branch from 5a16835 to e986dbc Compare July 28, 2019 16:34
@Jibbarth Jibbarth marked this pull request as ready for review July 28, 2019 16:48
@Jibbarth Jibbarth requested a review from nunomaduro July 28, 2019 16:49
Copy link
Collaborator

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

Looks good!

Made some small comments, however haven't tried running it locally yet.

docs/insights/README.md Outdated Show resolved Hide resolved
docs/insights/architecture.md Outdated Show resolved Hide resolved
docs/insights/architecture.md Outdated Show resolved Hide resolved
docs/insights/architecture.md Outdated Show resolved Hide resolved
docs/insights/architecture.md Outdated Show resolved Hide resolved
docs/insights/code.md Outdated Show resolved Hide resolved
src/Application/Adapters/Drupal/Preset.php Outdated Show resolved Hide resolved
@Jibbarth
Copy link
Collaborator Author

馃槄 I forgot to handle exclude file in config option. I have to add it

Copy link
Collaborator

@olivernybroe olivernybroe 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 see the changes after we removed ECS 馃憤 It looks awesome.

src/Domain/Contracts/FileProcessor.php Show resolved Hide resolved
src/Domain/Insights/SniffDecorator.php Outdated Show resolved Hide resolved
*/
public function addSniffs(array $sniffs): void
public function addInsights(array $insights): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks clean, nice solution.

@Jibbarth
Copy link
Collaborator Author

@olivernybroe I refactored a bit how InsightFactory load Sniffs and Fixers insights with an InsightLoader, that are provided from Container thanks to a tag.

That avoid adding custom code in InsightFactory later when we want add new Insights (from phpstan for example). In the same way, filesprocessors are now retrieved by tag also.

That means if we want add an other file processor, less work is required. Just create an specific InsightLoader, a specific FileProcessor, and inject them in config/container.php

WDYT ?

Copy link
Collaborator

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

I love these changes! 鉂わ笍

This is a perfect way to do it, thank you 馃憤

src/Domain/Insights/FixerDecorator.php Outdated Show resolved Hide resolved
src/Domain/Insights/InsightFactory.php Outdated Show resolved Hide resolved
src/Domain/Contracts/InsightLoader.php Outdated Show resolved Hide resolved
@nunomaduro
Copy link
Owner

I am so excited to have this.

@olivernybroe
Copy link
Collaborator

@Jibbarth Perfect I think it is ready to get merged.

@nunomaduro WDYT?

@nunomaduro
Copy link
Owner

Let me make a quick review.

Copy link
Owner

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

Alright, @olivernybroe the pull request seems to big for me. I trust you on this one.

@nunomaduro
Copy link
Owner

Just to make it clear - how exactly we pass the option to fix the stuff ?

@olivernybroe
Copy link
Collaborator

@nunomaduro, there are no fix option in this pr, it only shows diffs. The fix option will be a feature later.

@Jibbarth you are welcome to merge it in 鈾ワ笍

@Jibbarth Jibbarth merged commit 9b0418e into nunomaduro:master Sep 23, 2019
@Jibbarth Jibbarth deleted the feature/php-cs-fixer-as-insights branch September 23, 2019 17:49
@Jibbarth
Copy link
Collaborator Author

馃帀 Thank you so mutch @olivernybroe for time you take reviewing this big PR !

@nunomaduro Yes, it's still not yet an autofixer for code, but only a diff when code should be improved. Autofixer may be the next step ? 馃檮 馃槄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants