-
-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report implement Error, and a few more suggestions #87
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! I think most of these changes are really beneficial, but I'm a bit concerned about moving the cache into the report builder (and hence adding the Reports are supposed to be abstract representations of an error and aren't supposed to carry file caches around with them (instead, it's up to the consumer of the error to provide the necessary source files to render them). I think I'd prefer it if the What do you think? |
Now that you mention about carrying caches i see that yeah that would easily build up memory & cloning once one starts stacking errors, so building the textual representation by just giving a reference to a cache would likely be the way to go 👍 I'm thinking maybe all its needed is to go to the report rendering path at I'm going to give it a shot later and see what i can do, thanks 🦖 |
…entially Report" This reverts commit a91e4c9.
…inish and render it, store the render in the Report
@zesterer Okay so i made roughly what i described above and would like for a idea review on it, i think its roughly on that direction. Mind that i didn't check somethings like the sanity of doing Also that error test can be a bit improved (and prints removed), but overall i consider these details for now. Let me know thoughts on the change |
<?xml version="1.0" encoding="UTF-8"?> | ||
<project version="4"> | ||
<component name="VcsDirectoryMappings"> | ||
<mapping directory="" vcs="Git" /> | ||
</component> | ||
</project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you could remove this file and add it to your local .gitignore
if necessary?
I'm sorry, but I'm not sure I can accept this PR. The API changes still require the cache to be present in the call to Further, I am fine, in general, with I appreciate the time that's been put into this PR, but the changes as they are right now are fundamentally incompatible with the intended use of the crate's API features. |
In light of the above rejection, It seems worth mentioning what people who want this should do instead in the current API. I believe that you can just call |
Hello hope y'all doing well 👋
I have seen that
ariadne::Report
did not implementError
andDisplay
and i thought it was a problem for some reasons:Also a detail about
ariadne::Cache
, i tried to make itClone
but i failed to address all the needs there with thedupe
function, and maybe it could implementClone
directly instead?Another thing is that for
Display
to work well it needs pre-acess toC: Cache
, so i took the liberty to move it intoReportBuilder
directly, which sequentially then goes toReport
(I also included a assert to be sure it tells the developer that it is needed), but i guess y'all might want this different, just an ideaI have tried to make some changes to address this, take this PR as more of a request since i'm unsure if i did it too well, but i think there is very few things to address to make this PR ok
I also took the liberty to move
write_to_string
impl from the tests intoReport
directly, as it its useful thereThank you for your time 🎈 🎈