Refactor shared TargetProblem data types into their own module. #6774
Conversation
| -- | The various error conditions that can occur when matching a | ||
| -- 'TargetSelector' against 'AvailableTarget's which can be extended | ||
| -- with command specific target problems as described by 'e'. | ||
| data ExtensibleTargetProblem e |
There was a problem hiding this comment.
Can TargetProblemCommon and ExtensibleTargetProblem be merged into one?
There was a problem hiding this comment.
That would be doable, would just require some more code shuffling. One question would be if we want to inline all of the cases of TargetProblemCommon into the top level ExtensibleTargetProblem or if we want to keep the hierarchy as is.
At the least we could move the TargetProblemCommon definition into TargetProblem.hs, I'd prefer to leave the change of flattening the hierarchy (if that's what we decide to do) to a follow-up PR to keep this one as simply extracting the repeated code into one location.
There was a problem hiding this comment.
Please shuffle code.
TargetProblemCommonis a good nameExtensibleTargetProblemis bad nameExtensibleTargetProblemNoTargetis also bad. First we rename it fromTargetProblemNoTargetto then rename back toTargetProblemNoTargetagain.
This commit is good start. But I won't merge it. Please at least try TargetProblemCommon extraction as well in follow up commit, not PR.
There was a problem hiding this comment.
See most recent commit, to summarize:
- Rename
ExtensibleTargetProblem->TargetProblem(and change alias) - Shorten data constructor names in
TargetProblem - Extracts
TargetProblemCommonintoTargetProblem.hs
PTAL!
This adds a new 'ExtensibleTargetProblem e' type which can be used uniformly across the commands, supporting the most common use case of errors being one of: 'common', 'none enabled', or 'no targets'. In cases where custom target errors are needed, the base set can be expanded with a custom problem type, and custom reporting/rendering is supported.
Also: - Rename ExtensibleTargetProblem -> TargetProblem - Rename TargetProblem alias -> TargetProblem' (following convention) - Shorted data constructor names it TargetProblem - Moved "problem rendering" to CmdErrorMessages module
m-renaud
force-pushed
the
m-renaud-target-problem-refactor
branch
from
7d9fe46
to
beefecc
Compare
|
|
||
| -- | Target problems that occur during project orchestration. | ||
| data TargetProblemCommon | ||
| = TargetNotInProject PackageName |
There was a problem hiding this comment.
3 spaces, cabal is truly diverse!
There was a problem hiding this comment.
Haha! I really wish we could just run a formatter over the whole code base (I don't care which one!) so that I can stop worrying about indentation.
|
Want me to squash commits in the meantime or will you handle that when you make the tweaks and merge later? |
|
merged as #6821. Thanks! |
This adds a new 'ExtensibleTargetProblem e' type which can be used uniformly
across the commands, supporting the most common use case of errors being one of:
'common', 'none enabled', or 'no targets'. In cases where custom target errors
are needed, the base set can be expanded with a custom problem type, and custom
reporting/rendering is supported.
Please include the following checklist in your PR:
changelog.ddirectory).Please also shortly describe how you tested your change. Bonus points for added tests!