zb Provide a better error message for a failed connection that uses an address #1480
Conversation
|
Thank you but please follow our contributing guideline. |
|
I apology for not having followed the guidelines. I will follow them and resubmit |
No worries. You're not the first one to miss them and you'll certainly not be the last one. :)
Why are you closing the PR then. Just (force) push to the same branch. |
|
@plrigaux ping? |
|
Sorry busy , maybe for this weekend |
…ing an" This reverts commit 1c4a8f6.
plrigaux
force-pushed
the
improve_zb_error
branch
from
b79fd3c to
d2f2f45
Compare
plrigaux
changed the title
|
I tried to follow the guidelines and push force it. Hope it is ok. |
Thanks for trying. You created 2 additional commits instead of modifying your original commit. Please rebase and squash them and add the emoji prefix (I've made that as easy as possible these days so no reason to skip it) while you're at it. Forced push is not a requirement on its own, you just need to do that when you modify the commits, which you have yet. |
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
Other than these minor adjustments and git commits that I pointed out earlier, LGTM otherwise. Thanks so much for doing this! 🙏
| /// An I/O error. | ||
| InputOutput(Arc<io::Error>), | ||
| /// An I/O error with the related URI. | ||
| InputOutputAddress(Arc<io::Error>, Address), |
There was a problem hiding this comment.
This a very strange name IMO that will confuse people. I actually suggested a specific name in #14778: zbus::Error::Connection. This error is about connection failures.
| Address(String), | ||
| /// An I/O error. | ||
| InputOutput(Arc<io::Error>), | ||
| /// An I/O error with the related URI. |
There was a problem hiding this comment.
Let's be a bit more descriptive here and document the arguments (especially since we don't name them in here, which I know is not ideal).
| .clone() //This clone is necessary to capture on error the address because the connect | ||
| // function consume the address object. |
There was a problem hiding this comment.
We could have the Address::connect do the mapping instead, or return Result<Stream, (io::Error, Self) (open to other options too) or maybe even have the Address::connect just not take ownership of address instance (it's internal after all).
Since this should not require a major effort and there is no urgency here, I think it makes sense to already avoid the cloning here, instead of leaving it for the future, which is likely never going to happen. :)
P.S. do remember the atomic commits rule, so please put the needed refactoring in a separate commit that comes before this one. 🙏
There was a problem hiding this comment.
We could have the
Address::connectdo the mapping instead, or returnResult<Stream, (io::Error, Self)(open to other options too) or maybe even have theAddress::connectjust not take ownership of address instance (it's internal after all).
Just FYI, I would very much prefer the first option here: have the Address::connect (or Transport::connect) do the mapping.
| Error::Address(e) => write!(f, "address error: {e}"), | ||
| Error::ExcessData => write!(f, "excess data"), | ||
| Error::InputOutput(e) => write!(f, "I/O error: {e}"), | ||
| Error::InputOutputAddress(e, address) => write!(f, "I/O error: {e} for {address}"), |
There was a problem hiding this comment.
This will need to be update too:
| Error::InputOutputAddress(e, address) => write!(f, "I/O error: {e} for {address}"), | |
| Error::InputOutputAddress(e, address) => write!(f, "failed to connect to `{address}`: {e}"), |
|
Good I will make the changes. Also I will look how I modify a commit (I'm not fluent in git) |
Ping? If you need help with git, please let me know and I can try to help out. We do have hints in our contribution guidelines how to modify commits btw. :) |
|
@plrigaux hello? If you don't have time for this anymore, please let me know. It'd be shame though since it's only some very minor things that are needed to get this merged. |
|
Hello,
the change is minor, I tried to submit a pr, but I'm not savvy in open
source multiple pr and it will take me time to learn how to do it.
I don't mind if you reject my pr
thank you
Pierre-Luc Rigaux
514 808 6241
…On Wed, Dec 17, 2025, 13:13 Zeeshan Ali Khan ***@***.***> wrote:
*zeenix* left a comment (z-galaxy/zbus#1480)
<#1480 (comment)>
@plrigaux <https://github.com/plrigaux> hello? If you don't have time for
this anymore, please let me know. It'd be shame though since it's only some
very minor things that are needed to get this merged.
—
Reply to this email directly, view it on GitHub
<#1480 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADX5JGULQWCBAZD2GFS54D4CGMMVAVCNFSM6AAAAACE3MFXAWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMNRWGU3TIMBQGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
That's quite a contradiction to your previous comment here, where you showed a positive attitude. I'm not sure what you mean even. You mostly had to make some very minor improvements to your work and it would have been good to go. Not much to learn here except maybe some essential of git (that are useful to learn in general) and I offered you help with that so you could have taken my offer and you could have focused only on the parts that require absolutely no learning from you.
If I wanted to do that, I wouldn't have spent time in reviewing the PR. Next time if you don't intend to address review comments, please help reviewer save time by being upfront about this. 🙏 |
2 participants
Fixes #1478
I created a new error, but I have got no choice to clone the address to send the error with the address information. Because the value is moved with the call of connect().
Maybe some refactoring can remove the necessity to use clone().