search

jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
252 stars 13 forks source link

Add Rule.filterErrorsForFiles #115

Closed jiegillet closed 2 years ago

jiegillet commented 2 years ago

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 commented 2 years ago

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...

jiegillet commented 2 years ago

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 commented 2 years ago

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 https://github.com/jfmengels/elm-review/pull/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 :thinking:

jiegillet commented 2 years ago

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

jfmengels commented 2 years ago

This is now released in v2.7.0!