Skip to content
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

Handle leftover issues from #683 #686

Open
wants to merge 10 commits into
base: rku/aql-machine
Choose a base branch
from

Conversation

jmg-duarte
Copy link
Contributor

@jmg-duarte jmg-duarte commented Mar 20, 2024

Handles:

  • if these errors are to be read by humans, they would be vastly more useful if they carried the context (i.e. the path inside the event payload object)
  • since these errors will usually not be read by humans and they are expensive to construct (string formatting), it would be really great if the error carried the information for printing but only did it in its Display impl

#683 (comment)

@jmg-duarte jmg-duarte requested a review from rkuhn March 20, 2024 15:48
@jmg-duarte jmg-duarte changed the title Convert anyhow errors to thiserror Handle leftover issues from #683 Mar 20, 2024
Copy link
Member

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a good first step! Now make it so that constructing a TypeError does not need allocation (i.e. no .clone(), .to_string() etc.) by only referencing the data needed for printing.

with_path is the only place where allocation really is required because of the dynamic nature. Here, it can be minimised by using a SmallVec and storing &'a Index (or a new enum if required).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants