-
-
Notifications
You must be signed in to change notification settings - Fork 414
refactor!: catch up Exn designs with the upstream #2384
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
Conversation
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
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.
Thanks a lot, that's much appreciated!
All looks good, but it looks like deref is required for convenient access to the error, but I am open to alternative solutions that don't add bloat/inconvenience to the site of creation.
Edit: I set it back into draft so I get a signal when it's deemed ready for review/merge.
This reverts commit 9bbe26a.
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
This is correct. I revert the change to keep the current solution. I'll consider whether to add this |
gix-error/src/exn/impls.rs
Outdated
|
|
||
| /// Use the current exception the head of a chain, adding `errors` to its children. | ||
| #[track_caller] | ||
| pub fn chain_iter<T, I>(mut self, errors: I) -> Exn<E> |
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.
Perhaps change this to chain_all as well.
P.S. I'll start a PR on exn to add these two chain methods. In the initial use case of exn in ScopeDB, we can always know all the children when constructing the chain/tree, so we doubt whether to make children mutable after the chain has been established. But reading gix's use cases, it seems chains that adding children afterwards can be helpful.
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.
Perhaps change this to chain_all as well.
That sounds great, please feel free to do this here, otherwise I will do it in a follow-up.
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.
Added.
This reverts commit 73cf39a.
Signed-off-by: tison <[email protected]>
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.
Now I know gix-error has to wrap once more the inner error into Untyped because downcast must have the type param matched. In this case, it seems to be inevitable.
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.
In theory, the into_inner() and into_box() can be removed as there is always gix_error::Error to hold these types.
Let's leave it as is for now, and I will consider removing them to avoid that double-box.
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
|
All tests passed. Welcome to drop another review. |
Byron
left a comment
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 great, thanks so much! It's my pleasure to have you as contributor!
Also, extra-thanks for the cleanup :).
|
Oh, may I ask to squash this PR into one commit? |
Sure. This is what I typically expected - Since we rely on GitHub, we use PR to track the dev history and squash commits for an easy-reading main linear commit history.
Agree. I'd change the PR title now. |
|
Sorry, I meant the commits. I can always squash via GH or rewrite history, but I'd put words into your mouth and destroy your signatures. If you don't mind, I can do that of course. |
Never mind. Please do that. |
This follows #2352 (comment).
I push several changes in separate commits. If you'd like to handle them in multiple PRs, please let me know.