Skip to content

Report errors if no files were found or if source-directories are empty #352

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

Merged
merged 3 commits into from
Feb 23, 2025

Conversation

jfmengels
Copy link
Owner

Supersedes #341, by addressing the root problem.

As @lishaduck pointed out in #341 (comment)

we don't have any tests where there isn't a source dir, do we? I have a sneaking suspicion that the reason it hung was that tinyglobby <0.2.11 would've scanned the entire drive when given an absolute path, but in case I'm wrong there, we should probably make sure elm-review doesn't hang in the actual situation where there aren't any files.

Indeed, when there are no Elm files at all, elm-review hangs. I added a test for it, and we're now exiting with an error should that happen. I also added a specific error (and test) for when source-directories is empty for some reason.

I also uniqued source-directories we're running, because there was some duplicates otherwise, and that was one additional glob call.

@jfmengels jfmengels merged commit a856c57 into main Feb 23, 2025
3 checks passed
@jfmengels jfmengels deleted the test-empty-project branch February 23, 2025 22:11
Comment on lines +56 to +58
const elmFilesToRead = [
...new Set(filesInDirectories.flatMap((directory) => directory.files))
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that is just unique() inlined. Maybe I'll de-inline it someday. Probably not. :)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah right. Yeah I reached out for unique after writing this code. I'll make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was rebasing and this made a conflict, so I fixed it in #344.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Just saw your message. I fixed it in #354, it's on master now 😅

Copy link
Contributor

@lishaduck lishaduck Feb 23, 2025

Choose a reason for hiding this comment

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

Already re-(re-?)rebased 🤣

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