cabal check: add typed errors#8269
Conversation
andreasabel
left a comment
andreasabel
left a comment
There was a problem hiding this comment.
Good next step (getting away from String errors towards structured errors)!
ffaf1
force-pushed
the
typed-errors
branch
3 times, most recently
from
291a877 to
d9d7d12
Compare
ffaf1
force-pushed
the
typed-errors
branch
5 times, most recently
from
17bb456 to
44d664b
Compare
ffaf1
changed the title
cabal check: add typed errors (very WiP)cabal check: add typed errors (WiP)
ffaf1
force-pushed
the
typed-errors
branch
8 times, most recently
from
fbc7820 to
8bd81ff
Compare
ffaf1
changed the title
cabal check: add typed errors (WiP)cabal check: add typed errors
| extraDocFiles :: [FilePath] | ||
| } | ||
| deriving (Generic, Show, Read, Eq, Typeable, Data) | ||
| deriving (Generic, Show, Read, Eq, Ord, Typeable, Data) |
There was a problem hiding this comment.
I had to add a number of Ord instances after structuring check errors (before: sorting was done on output Strings). It seems to me the new deriving (Ord) will not cause problems (most of the types are sum types).
|
|
||
| instance Show PackageCheck where | ||
| show notice = explanation notice | ||
| show notice = ppExplanation (explanation notice) |
There was a problem hiding this comment.
I don’t like Show instances that cannot be Read back, but since this is a public interface (used by hackage-server, stack etc.) I did not modify this behaviour.
| # Setup configure | ||
| Configuring build-tool-depends-missing-0.1.0.0... | ||
| Error: setup: The package depends on a missing internal executable: | ||
| Error: setup: The package depends on a missing internal executable: build-tool-depends-missing:hello-world |
There was a problem hiding this comment.
Introducing MissingInternalExe highlighted a bug, I corrected it and accepted the new test output.
| # cabal check | ||
| Warning: The following errors will cause portability problems on other environments: | ||
| Warning: The 'ghc-options' contain the path 'dist/file' which points inside the 'dist' directory. This is not reliable because the location of this directory is configurable by the user (or package manager). In addition the layout of the 'dist' directory is subject to change in future versions of Cabal. | ||
| Warning: 'ghc-options' path 'dist/file' points inside the 'dist' directory. This is not reliable because the location of this directory is configurable by the user (or package manager). In addition the layout of the 'dist' directory is subject to change in future versions of Cabal. |
There was a problem hiding this comment.
Minor rewording due to refactoring two string errors in one data constructor.
| Warning: In 'data-files': the pattern 'non-existent-directory/*.dat' attempts to match files in the directory 'non-existent-directory', but there is no directory by that name. | ||
| Warning: In 'extra-source-files': the pattern 'dir/*.html' does not match any files. | ||
| Warning: In 'extra-source-files': the pattern 'dir/*.html' does not match the file 'dir/foo.en.html' because the extensions do not exactly match (e.g., foo.en.html does not exactly match *.html). To enable looser suffix-only matching, set 'cabal-version: 2.4' or higher. | ||
| Warning: In 'data-files': the pattern 'non-existent-directory/*.dat' attempts to match files in the directory 'non-existent-directory', but there is no directory by that name. |
There was a problem hiding this comment.
Output is the same, lines are sorted differently (before: output string sorting).
|
Ready for review, the 112 Meat of the patch is |
jneira
left a comment
jneira
left a comment
There was a problem hiding this comment.
Looks really good, many thanks
| unless (null errors) $ | ||
| notice verbosity $ "Distribution quality errors:\n" | ||
| ++ unlines (map explanation errors) | ||
| ++ unlines (map show errors) |
There was a problem hiding this comment.
Isn't introducing show here backwards? The aim should be to reserve show for Haskell representations of data, and use pretty or the like for user-friendly display.
There was a problem hiding this comment.
Indeed indeed! I would have removed that Show instance and explanation accessor which is at risk of becoming partial in the future.
I did not do it because the latter that would break hackage-server (they use explanation) and the former (maybe) other packages depending on Cabal (hackage-server? stack? some IDEs? — I cannot be sure about that because hunting shows in the wild is not as easy).
If we collectively decide it is a price worth paying, I will gladly do it! Indeed a Show that cannot be Read back is not haskelly at all!
| if null errors | ||
| then traverse_ (warn verbosity) warnings | ||
| else die' verbosity (intercalate "\n\n" errors) | ||
| then traverse_ (warn verbosity) (map show warnings) |
There was a problem hiding this comment.
See below. Show is for Haskell concrete syntax of data, for displaying to user use pretty.
There was a problem hiding this comment.
Ah, reading better, I undestand now: «do not introduce show in internal code». That is very reasonable. I can make a function for principled pretty printing and use it internally, if we decide to deprecate that Show instance later we can do it without rewriting some of the implementation.
Will make a PR this evening or tomorrow.
4 participants
Towards #8211
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!