feat: ✨ explain() errors
#208
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hopefully makes errors easier to read
joelostblom
commented
Nov 21, 2025
Contributor
Author
|
@seedcase-project/developers Just a draft for now and lots of things to fix, but early input welcome from anyone if this approach is already going down the down route. The output currently looks like this: 
|
lwjohnst86
requested changes
Nov 21, 2025
Member
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Super cool 🎉 Some small comments so far 🖊️
Moves all logic out of init() and instead into two separate functions: one for iterating over the errors and adding a general message, and one for formatting the message for each issue.
Member
|
@joelostblom build fails though! |
lwjohnst86
requested changes
Nov 24, 2025
Member
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Nice! Very minor comment about naming
To avoid having to specify it everywhere where it is not used.
joelostblom
commented
Nov 24, 2025
lwjohnst86
requested changes
Nov 24, 2025
lwjohnst86
requested changes
Nov 24, 2025
For certain error types, a dict or list is set as the instance, which means logic elsewhere that calls `set()` on `Issue` fails.
lwjohnst86
reviewed
Nov 24, 2025
lwjohnst86
reviewed
Nov 24, 2025
Since we perform deduplication of Issues later, we need to either only allow hashable items or write custome comparison logic to compare unhashable ones. Technically, writing custom comparison logic that ignores unhashable values might be the most theoretically appropriate (since it is redundant to compare `instance` when also comparing `jsonpath`), but it could look convoluted and have worse performance. This is a simpler solution that is easy to understand and that does not involve any significant performance penalty as the extra comparison is of small items. Trying this out as a first approach and changing if we notice that there are occasions where we would value having access to an unhashable instance. For more discussion, see: #208 (comment)
lwjohnst86
requested changes
Nov 25, 2025
Member
lwjohnst86
left a comment
lwjohnst86
left a comment
There was a problem hiding this comment.
Look super nice! Just some small comments about docstrings.
joelostblom
added a commit
that referenced
this pull request
Nov 26, 2025
But always include it in the Issue object instead of sometimes excluding it based on whether it is hashable or not. See further discussion in #208 (comment)
But always include it in the Issue object instead of sometimes excluding it based on whether it is hashable or not. See further discussion in #208 (comment)
joelostblom
force-pushed
the
feat/explain-errors
branch
from
d707704 to
3c4798b
Compare
Contributor
Author
|
@lwjohnst86 @martonvago I think I have addressed everything we discussed so far and tests are passing so I'm marking this as ready for a review! A few things I intentionally left out:
|
martonvago
previously requested changes
Nov 26, 2025
Contributor
martonvago
left a comment
martonvago
left a comment
There was a problem hiding this comment.
Really nice ⭐!!
I added some comments, feel free to ignore what you don't care about
Co-authored-by: martonvago <57952344+martonvago@users.noreply.github.com>
joelostblom
commented
Nov 27, 2025
Contributor
Author
joelostblom
left a comment
joelostblom
left a comment
There was a problem hiding this comment.
Thanks @martonvago ! I went through and tried to address your comments.
This was referenced Nov 27, 2025
github-project-automation
bot
moved this from In Progress
to Done
in Iteration planning
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Creates a more human readable format to explain error messages
Closes #11, closes #58, closes #14
Needs an in-depth review.
Checklist
just run-all