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 Rule.filterErrorsForFiles #115

Merged
merged 12 commits into from
Feb 4, 2022

Conversation

jiegillet
Copy link
Contributor

As discussed on Slack, here is my proposal for adding Rule.onlyAllowErrorsForSpecifiedDirectories and Rule.onlyAllowErrorsForSpecifiedFiles.

When used several times, it adds to an internal list of "specified" files/directories, I tried to find a name that reflected that, that's why it ended up being a bit verbose. I considered other options, like taking the intersections of specified files, but in that case, rules would run on less files and have less chance of being noticed if several instances of nlyAllowErrorsForSpecified* were used independently by mistake.

When used in combination with ignoreErrorsFor* it still only allows the specified locations and then ignore files from those.

I ended up reverting the internal logic in Exceptions (renamed addFiles as ignoreFiles) because it was confusing.

I didn't feel a need to add configuration errors, in this case, the worst that can happen is people ignoring files that are not allowed anyway. In any case, I'm happy to discuss more and make changes if the API doesn't feel natural to use, or if my code can be improved.

@jiegillet
Copy link
Contributor Author

jiegillet commented Jan 22, 2022

I've switched to using filterErrors. That'll teach me to code things up before the design is fixed haha (for the record, you fully warned me!).

I'm not sure what to do about the conflict in the PR. When pulled, the code shows docs.json on a single line, but only here it shows multiple lines...

Copy link
Owner

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

This is looking pretty good @jiegillet, thanks a lot for your work!

I have a few comments, feel free to let me know if you disagree or have better solutions in mind. As for the docs.json file, make sure you're rebased on top of master, and then just re-generate the file to overwrite it. As for the multiline thing, it should be on one line. My guess is that you may have manually saved (and therefore reformatted it) in your editor maybe(?) in your previous PR about typos.

src/Review/Exceptions.elm Outdated Show resolved Hide resolved
src/Review/Rule.elm Outdated Show resolved Hide resolved
src/Review/Rule.elm Outdated Show resolved Hide resolved
src/Review/Rule.elm Outdated Show resolved Hide resolved
@jiegillet jiegillet force-pushed the jie-onlyForDirectories branch from add6f56 to ffc2fad Compare January 22, 2022 09:26
@jiegillet
Copy link
Contributor Author

All comments addressed (and fixed CI failure), thank you for the review!

The issue with the docs is that elm make produces a one-line JSON, so I had to format it first before matching to the master version. (fixing that was a bit messy, that's why I had to force push).

@jfmengels
Copy link
Owner

The issue with the docs is that elm make produces a one-line JSON,

Actually it should be on one line. The multiline was introduced in #116 and I hadn't noticed it.


I'm wondering about the naming of this function. Since this function applies a predicate on files, not errors, I don't think it makes sense to call it filterErrors. What do you think about filterErrorsFor or filterErrorsForFiles (filterErrorForFile?)? I'll ask around what people think as well 🤔

@jiegillet
Copy link
Contributor Author

Ah! #116? Then it's my fault, sorry -_-
I'll fix it!

Copy link
Owner

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @jiegillet
Thanks a lot for your hard work here 🙏

@jfmengels jfmengels changed the title Only allow reported rules for specific directories or files Add Rule.filterErrorsForFiles Feb 4, 2022
@jfmengels jfmengels merged commit 2bc6c2e into jfmengels:master Feb 4, 2022
@jfmengels
Copy link
Owner

This is now released in v2.7.0!

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