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

Add regex support and umbrella test path regex to excluded_paths for ModuleDependencies. #706

Merged

Conversation

@TheFirstAvenger
Copy link
Contributor

TheFirstAvenger commented Oct 15, 2019

Adds umbrella app test directories to default ignore in ModuleDependencies check when in umbrella app.

Fixes #705.

@rrrene

This comment has been minimized.

Copy link
Owner

rrrene commented Oct 17, 2019

Thank you for putting this together. 👍

I am unfortunately not sure how this fits with Credo's existing code/behaviour: The test/ folder in the project's root is explicitly included, but your PR would exclude the umbrella app's test/ folders?

Could you clarify what your intentions are?

@TheFirstAvenger

This comment has been minimized.

Copy link
Contributor Author

TheFirstAvenger commented Oct 17, 2019

@rrrene this PR is focused specifically on the functionality in the ModuleDependencies check.

My understanding of the ModuleDependencies check is that it warns if a single module is reaching out to too many other modules, as it indicates that the module is doing to much and should be broken up. The problem with this check is that tests legitimately need to reach out to a number of different other modules for things like factories, mocks, etc... so they would often fail this check. To avoid these false positives, by default, the test directory is ignored by this check.

While the check ignores the root test folder, it doesn't currently ignore test in the umbrella child apps, and therefore umbrella test modules currently fail this check. This PR makes the check act the same towards umbrella test directories as it does single app test directories (i.e. it ignores them).

@rrrene

This comment has been minimized.

Copy link
Owner

rrrene commented Oct 17, 2019

Ah, you are right, I see. 👍

Wouldn't changing @default_params[:excluded_paths] to also accept regexes (and making the default value ~r"/test/") also accomplish that and make the logic more straightforward? We already accept string and regexes in other configs, so this seems a bit more consistent to me - while still achieving your original goal. Thoughts?

@TheFirstAvenger TheFirstAvenger force-pushed the TheFirstAvenger:mb-module-dependencies-umbrella branch from aab804d to ae77f0f Oct 17, 2019
@TheFirstAvenger

This comment has been minimized.

Copy link
Contributor Author

TheFirstAvenger commented Oct 17, 2019

@rrrene Good idea, much simpler. I have updated the code to accept regexes in addition to the strings it currently accepts.

@TheFirstAvenger TheFirstAvenger changed the title Add umbrella app test folders to default ignore in ModuleDependencies. Add regex support and umbrella test path regex to excluded_paths for ModuleDependencies. Oct 17, 2019
@rrrene rrrene merged commit da1d940 into rrrene:master Oct 18, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rrrene

This comment has been minimized.

Copy link
Owner

rrrene commented Oct 18, 2019

@TheFirstAvenger 👍 Thx!

@TheFirstAvenger TheFirstAvenger deleted the TheFirstAvenger:mb-module-dependencies-umbrella branch Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.