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

Feature/exclude directory for insights #293

Merged

Conversation

Jibbarth
Copy link
Collaborator

@Jibbarth Jibbarth commented Oct 19, 2019

Q A
Bug fix? no
New feature? yes
Fixed tickets #240 #243 #108 (comment)

This PR add possibility to configure exclude part with a directory in addition of filepath:

        ForbiddenSetterSniff::class => [
            'exclude' => [
                'src/Domain/MySpeciedClass',
                'src/Models',
            ],
        ],

In the example above, all classes in src/Models will be excluded for the ForbiddenSetterSniff

This PR also port the exclude feature to our own Insights (ForbiddenDefineGlobalConstants, ForbiddenGlobals ...). I added the exclude check for each Insights that implement HasDetail interface.

@Jibbarth Jibbarth added the enhancement label Oct 19, 2019
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe left a comment

I have commented on some small improvements for readability and performance :)

src/Domain/Insights/CyclomaticComplexityIsHigh.php Outdated Show resolved Hide resolved
src/Domain/Helper/FilesFinder.php Outdated Show resolved Hide resolved
src/Domain/Insights/FixerDecorator.php Show resolved Hide resolved
src/Domain/Insights/ForbiddenFinalClasses.php Show resolved Hide resolved
src/Domain/Insights/Insight.php Show resolved Hide resolved
@olivernybroe
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe commented Oct 21, 2019

We should maybe consider changing our preset after this gets merged. I think we could help the Laravel developers at least with some changes here.

@Jibbarth Jibbarth force-pushed the feature/exclude-directory-for-insights branch from 079a48e to 5b39da7 Compare Oct 28, 2019
@Jibbarth Jibbarth requested a review from olivernybroe Oct 28, 2019
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe left a comment

Alright, I think this is almost ready now.

There is a duplication on a method which I think we should handle, either with a trait, a helper class or just a adding the method to our insight class.

src/Domain/Insights/CyclomaticComplexityIsHigh.php Outdated Show resolved Hide resolved
src/Domain/Insights/ForbiddenDefineGlobalConstants.php Outdated Show resolved Hide resolved
src/Domain/Insights/ForbiddenFinalClasses.php Outdated Show resolved Hide resolved
src/Domain/Insights/ForbiddenNormalClasses.php Outdated Show resolved Hide resolved
src/Domain/Insights/ForbiddenTraits.php Outdated Show resolved Hide resolved
$exclude = $config['exclude'] ?? [];
if (count($exclude) > 0) {
$this->excludedFiles = Files::find(
(string) getcwd(),
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe Oct 29, 2019

Choose a reason for hiding this comment

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

How does this act in case where getcwd returns false?

Copy link
Collaborator Author

@Jibbarth Jibbarth Oct 29, 2019

Choose a reason for hiding this comment

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

I tried and it throw a DirectoryNotFoundException.

I can add a check on $basedir in Files::find and return an empty array if it's not exist to avoid throw an exception ? WDYT

Copy link
Sponsor Collaborator

@olivernybroe olivernybroe Nov 4, 2019

Choose a reason for hiding this comment

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

Hmm whatever you prefer here tbh. I don't really have an opinion

src/Domain/Insights/InsightFactory.php Outdated Show resolved Hide resolved
src/Domain/Insights/FixerDecorator.php Outdated Show resolved Hide resolved
Copy link
Sponsor Collaborator

@olivernybroe olivernybroe left a comment

Alright, when you have figured out which way you want to handle that weird edgecase with cwd, then you can go ahead 👍

The code is really nice, great work! 😃

@Jibbarth Jibbarth force-pushed the feature/exclude-directory-for-insights branch from d948c30 to f595789 Compare Nov 16, 2019
@Jibbarth
Copy link
Collaborator Author

@Jibbarth Jibbarth commented Nov 16, 2019

@olivernybroe I add a fallback when getcwd return false to use configuration Directory 😉

@Jibbarth Jibbarth merged commit 261e151 into nunomaduro:master Nov 16, 2019
1 check passed
@Jibbarth Jibbarth deleted the feature/exclude-directory-for-insights branch Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants