cabal check: add --ignore option

[ffaf1]

Add --ignore option to cabal check, so you can ignore specific warnings. Closes #8587.

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• Manual QA notes have been included.
• Tests have been added.

---

QA Notes

• cd into a new folder
• cabal init -nm
• cabal check
• inspect the output
• cabal check --ignore=no-category
• the specific warning is gone.

[mpickering]

I think there should be an explicit conversion function from the CheckExplanation to a "flag string" rather than displaying the constructor name directly.

Ideally all the check reasons would also be documented referring to these flag strings as well (and also ideally error codes for HF error index).

In the end I think it would be better to display something like..

[missing-upper-bounds] On library, these packages miss upper bounds:

or

On library, these packages miss upper bounds: [missing-upper-bounds]

I also think that passing an unknown option should just be a warning, rather than an error, so it's easier to use with different versions of cabal which might support different check flags.

[mpickering]

See my other comments

[ffaf1]

Ok, now:

• ignores are sandwitched between square brackets.

• ignores have their own names, and not names from constructors, e.g.

  Warning: [no-category] No 'category' field.
  

  (test on desirable ignores properties added).

• unrecognised ignores will emit a warning, not an error (test added).

• documentation has been updated with the list of ignores.

[mpickering]

I will look again tomorrow.

[mpickering]

Looks good to me.

The only question I have is the value of having CheckExplanationId as a datatype rather than type CheckExplanationId = String/ByteString/Text. It seems that it is only converted to a string, and parsed from a string, are there any other manipulations of it that I am missing?

[ffaf1]

tl;dr: we need it to output a warning on unrecognised ignore strings.

To output a warning when an unrecognized ignore string is passed to cabal check — say a spelling error like cabal check --ignore=msising-upper-bounds — I need a set of all valid string values and then call "msising-upper-bounds" `elem` allCheckStrings.

With CheckExplanationId as a datatype (instance of Enum and Bounded), I can build that set with map ppCheckExplanationId ([minBound..maxBound] :: CheckExplanationId). Not only that, but when we add constructors to CheckExplanation, compiler warnings pops up if we forgot to add the relevant CheckExplanationId

If CheckExplanationId were to be a String, we could not use no more the [minBound..maxBound] trick; instead we would have to manually manage a allCheckExplanationIds list, and remember to modify it each time we modify or add a constructor to CheckExplanation. This is error prone as the compiler will not spot mismatches.

Does this make sense?

[ffaf1]

@mpickering does the explanation satisfy you? I am happy to answer questions or rework the code if needed.

[ffaf1]

@mpickering I will ping again. If there is anything I can do to help this review, I will happily do so.

Comments addressed

[ulysses4ever]

@mpickering wasn't responsive recently. Maybe it's the holiday season... But given a very convincing explanation about the data type, I took the liberty to dismiss Matt's request for changes. We still need a second approval though...

[geekosaur]

FWIW @mpickering just said a few hours ago in #ghc:matrix.org that he's been very sick recently and not doing any work.

[ffaf1]

Thanks Artem and get well soon Matthew.

[ulysses4ever]

Great work, thank you! Nothing blocking from my perspective. So, it's up to you how to proceed.