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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Refact] configuration class #283

Merged

Conversation

Jibbarth
Copy link
Collaborator

@Jibbarth Jibbarth commented Oct 6, 2019

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

In order to fix this comment, I add a Configuration Class that carry config provided by user.

The Configuration class is resolved when container boot, so we can use DependencyInjection or container to retrieve configuration.
So we don't have anymore the need to manually pass config everywhere we need it.

As phpcsfixer came with it, I use symfony/options-resolver to improve configuration validation.

I think this refact is needed, because the config file is growing, and we should have a simpler way to validate it, and also retrieve it.

WDYT ?

@Jibbarth Jibbarth added the enhancement New feature or request label Oct 6, 2019
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.

Wauw, this is really clean, I love it ❤️

I think this will allow us to do a lot of extra stuff soo much easier in the future.

I only had a few comments about some naming, but else it just looks spot on

src/Application/DirectoryResolver.php Show resolved Hide resolved
src/Domain/Configuration.php Outdated Show resolved Hide resolved
src/Domain/Configuration.php Outdated Show resolved Hide resolved
Resolve configuration when loading container
Use dependency injection for configuration
@Jibbarth Jibbarth force-pushed the feature/configuration-class branch from 666e5f1 to e6a37e6 Compare October 7, 2019 11:10
@Jibbarth
Copy link
Collaborator Author

Jibbarth commented Oct 7, 2019

@olivernybroe Do you think you could test it in a Laravel application before merging ?

To be sure the php artisan insights still works, especially when we pass a directory argument ?

@olivernybroe
Copy link
Collaborator

@Jibbarth Did not work in Laravel, gave the following error

 Illuminate\Contracts\Container\BindingResolutionException  : Unresolvable dependency resolving [Parameter #0 [ <required> array $config ]] in class NunoMaduro\PhpInsights\Domain\Configuration

@Jibbarth Jibbarth force-pushed the feature/configuration-class branch from cc761f4 to 1b7846d Compare October 7, 2019 18:47
@Jibbarth
Copy link
Collaborator Author

Jibbarth commented Oct 7, 2019

@olivernybroe I think it's fixed now. Can you check the last commit and tell me if it looks correct for you ?

😅 I made my first Laravel install to fix this 🙌

@olivernybroe
Copy link
Collaborator

@Jibbarth

It works, nicely done! 😄

Congratz on Laravel, now you gotta use it all the time ;)

But, thanks for the refactor! ❤️

@olivernybroe olivernybroe merged commit 8a60fbc into nunomaduro:master Oct 8, 2019
@nunomaduro
Copy link
Owner

Thanks @Jibbarth for your work on this.

@Jibbarth Jibbarth deleted the feature/configuration-class branch October 12, 2019 08:58
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