ANE-1254: Error Structure #1364
Conversation
|
There was a pretty big change in this pr that changed the way we render errors to our diagnostic. Instead of rendering everything as Doc AnsiStyle, it has been changed to return an Errata object. Currently, we aren't necessarily doing much with the Errata module aside from using the source location and basic rendering features. However, Errata allows you to highlight code blocks and add styling using pointers / underlines. I think that this might be beneficial in cases where we come across errors when parsing things like our fossa-deps / .fossa config files. This would work probably need to done later down the line. I would like to get thoughts on this implementation and if it's too big of a change. |
|
Additionally, we used to capture contextual information about error failures through our errCtx carrier. It held a lot of information such as troubleshooting, documentation references, and other contextual information. In order, to structure and label our errors similar to broker. I created additional carriers for errHelp, errDoc, and errSupport to allow us to easily categorize the various types of contextual info that can be attached to an error. |
There was a problem hiding this comment.
This is a great change, and I love the look of the new error structure.
However, I have a few questions:
-
Is the use of
ImplicitParamsreasonable. I'd like to get a sanity check from someone else on the team on this. If you've already run this by someone else, then that counts as a sanity check -
Are the colour codes in
scan-summary.txtokay, or should we strip them out when writing toscan-summary.txt -
Is there any way we can add tests for this change. As I said in the comment below, I'm fine with these coming in a separate PR
I'd like to get answers on these before I approve. But it's looking great other than those questions!
| @@ -6,7 +6,8 @@ import Control.Carrier.Telemetry (withoutTelemetry) | |||
| import Control.Effect.Diagnostics (ToDiagnostic (..), fatal) | |||
| import Data.Text (Text) | |||
| import Effect.Exec (ExitCode (..)) | |||
| import Effect.Logger (ignoreLogger, logInfo, pretty) | |||
| import Effect.Logger (ignoreLogger, logInfo) | |||
There was a problem hiding this comment.
This is a scarily small ratio of test change to code change.
It could be that there are a bunch of tests for error cases but the changes in this PR don't affect those tests. In that case, the error code is being exercised by the tests and all is good.
If there are no tests on most of these errors currently, we should fix this up. I'm fine with doing that in a separate PR, as this one is huge.
If you've thought about this and come to the conclusion that this is too hard, that's reasonable, but it should be documented in the PR description.
There was a problem hiding this comment.
Yea I'm going to create a separate ticket to add more tests. I agree, I was pretty shocked when I saw the state of our test coverage here.
|
More diagnostic test cases will be added in a separate ticket! |
Overview
As a fossa user, errors should be structured.
Acceptance criteria
Testing plan
Here are how errors are structured now:
You can also test by triggering an error.
Risks
Metrics
References
Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an# Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.