Change sbot::init to not panic and instead return an error #51

notplants · 2022-07-12T11:14:47Z

notplants commented

2022-07-12 11:14:47 +00:00

not panicing is required for the error handling I'm building in peach_config::wait-for-sbot and is I think a better practice in general

notplants added 2 commits 2022-07-12 11:14:48 +00:00

e9667a57be Change sbot::init to not panic and instead return an error

48beb4a2e5 Merge branch 'main' of https://git.coopcloud.tech/glyph/golgi into new-rexport

notplants requested review from glyph 2022-07-12 11:30:52 +00:00

glyph approved these changes 2022-07-12 15:42:04 +00:00

glyph left a comment

Great. A long-overdue addition. Thanks!

glyph requested changes 2022-07-14 08:26:24 +00:00

glyph left a comment

Made a comment about error variants.

src/error.rs Outdated

						
				@ -35,2 +35,4 @@

				    /// Go-sbot error.

				    Sbot(String),

				    /// Error initializing Go-sbot.

				    SbotInit(String),

glyph commented

2022-07-14 08:25:55 +00:00

Taking a second look at this: I suggest using the existing Sbot(String) error variant rather than creating an additional variant (SbotInit(String)).

I've found it easier for maintenance to keep a smaller number of variants which bundle together errors of like kind.

Taking a second look at this: I suggest using the existing `Sbot(String)` error variant rather than creating an additional variant (`SbotInit(String)`). I've found it easier for maintenance to keep a smaller number of variants which bundle together errors of like kind.

notplants commented

2022-07-15 09:07:32 +00:00

Agreed I like having fewer variants too. Even having more generic error types, and using the string "message" within the error for giving more details, could even help debug, such that the the specific string in the "message" can help debug exactly where the error comes from.

👍 1

notplants commented

2022-07-15 09:13:20 +00:00

in this case I separated them initially because the Sbot error type said "sbot returned an error response",
which is not technically what these sbot intialization errors are. so I changed that error message template too to combine them

in this case I separated them initially because the Sbot error type said "sbot returned an error response", which is not technically what these sbot intialization errors are. so I changed that error message template too to combine them

❤️ 1

notplants added 1 commit 2022-07-15 09:17:16 +00:00

b24f2e4a06 Combine Sbot and SbotInit errors

glyph approved these changes 2022-07-15 09:54:23 +00:00

glyph left a comment

Thanks for the update. I like the improvement you made to the main Sbot error message.

🙏 🖤

Thanks for the update. I like the improvement you made to the main `Sbot` error message. 🙏 🖤

notplants merged commit 3c37297018 into main

2022-07-16 05:57:36 +00:00

notplants deleted branch no-panic

2022-07-16 05:57:37 +00:00