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.
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
Add error::Report type #90174
Add error::Report type #90174
Changes from 1 commit
59df6c8
6a59d0e
c6de413
c0f14cb
aa853bd
d2f49ee
32bcb81
840cb00
edd65f7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think we have any 'builder pattern' functions in std right now that take and return self by value. Instead, they take
&mut self
and return&mut Self
. That way, you can also use them without chaining them, as they modify the original object, but also allow chaining.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.
it's definitely preferable to have this one done by value, because otherwise you can end up with common usages breaking. Our original version used
&mut Self
but broke in the following trivial example:Report is designed so that you're always going to create it and then use it immediately, so being able to mutate a pre-defined report is never going to be useful.
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.
Then why is it a problem that your example breaks, if you're not supposed to store it for later use?
Your example makes me think that it would be useful to be able to do things like:
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.
I'm not sure I agree, I'd write that like this:
The issue to me is that I don't want to be forced to add a bunch of extra lines or inline the entire thing in a println in order to have it compile, if it it was returned by reference my example could only be written like this:
or
In my experience using
Report
so far I have found that it tends to be more annoying to use when it returned&mut Self
and when I inlined the method calls they often ended up having to be spread over multiple lines and made the code look much harder to read for me.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.
At the very least, it seems like
show_backtrace
andpretty
should be reverted to accept a boolean (we had this in a previous iteration, but ended up removing it).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.
(This is why in the other cases the builder type is different type than the type you end up building. Then the builder type doesn't need to contain any references or resources yet, such that
(&mut self) -> &mut Self
can be used, and only an.open()
,.spawn(..)
, or w/e at the end will make the object you wanted to make, which might end up borrowing things into the newly created object.But having two types in this case might get a bit annoying.)
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.
Yea, I think its particularly important that
fallible_fn().map_err(Report::new).unwrap()
be usable, so forcing a.build()
call at the end feels a little awkward to me.