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

Fix the path for included files in umbrella apps #1133

Merged
merged 2 commits into from
Jun 2, 2024
Merged

Fix the path for included files in umbrella apps #1133

merged 2 commits into from
Jun 2, 2024

Conversation

tomasz-tomczyk
Copy link
Contributor

Hello! I ran into an issue where Credo.Check.Refactor.PassAsyncInTestCases would not flag issues when running mix credo from the root of an umbrella application, but cd-ing into apps/app_web and running it from there would correctly flag the missing async option.

We only have a single .credo.exs config at the root and I replicated the issue with the barebones config:

%{
  configs: [
    %{
      name: "default",
      checks: %{
        enabled: [{Credo.Check.Refactor.PassAsyncInTestCases, []}]
      }
    }
  ]
}

Before this change, inside the Credo.Sources.find_in_dir the value for included_patterns would compile down to:

["/Users/tom/my_umbrella_app/test/**/*_test.exs"]

But that would skip everything that was under my_umbrella_app/apps/**/test

I see this was intentionally changed in ac76775 so maybe I'm wrong to submit this PR and would appreciate feedback! Changing this locally did make the check work from both the root and the individual umbrella app directories though.

@tomasz-tomczyk tomasz-tomczyk changed the title Fix the path for included files for umbrella apps Fix the path for included files in umbrella apps May 24, 2024
@rrrene
Copy link
Owner

rrrene commented May 25, 2024

Thx for pointing this out. I will take a closer look soon. 👍

@tomasz-tomczyk
Copy link
Contributor Author

Thanks for taking a look!

I've reproduced in a minimal example in this repo: https://github.com/tomasz-tomczyk/credo-async-example

@rrrene rrrene merged commit 980f56e into rrrene:master Jun 2, 2024
20 checks passed
rrrene added a commit that referenced this pull request Jun 2, 2024
@rrrene
Copy link
Owner

rrrene commented Jun 2, 2024

@tomasz-tomczyk I had to change the way the files were addressed, because the check for wrong file extensions would otherwise yield issues for files like lib/foo/foo_case_test.ex which we avoided in the past.

Please check out the RC to see if this works for you: https://github.com/rrrene/credo/releases/tag/v1.7.7-rc.0 👍

@tomasz-tomczyk
Copy link
Contributor Author

Ah okay, got it. Thanks! It works for us yes 👍

@tomasz-tomczyk tomasz-tomczyk deleted the fix-files-included branch June 13, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants