Refactor

oddlid commented

2026-01-16 01:25:44 +00:00

Contributor

Here’s some things I’d like to do, if you approve?

Replace as much as possible of package variables and init() functions with package ”constructors” and struct instances. This makes it a lot easier to test pieces in isolation, and also makes it possible to run tests in parallel without risk of race conditions.
Merge the top level context with signal handling, for cleaner / easier shutdown handling. This context should be passed to all goroutines, so they can exit cleanly on shutdown signals, and not leak. Also use this context during sleep, so one doesn’t have to wait for the sleep to finish, if sending a quit signal.
Split up large functions like main in smaller ones and return errors from those who need to instead of panicking all over the place, and then only have one top level panic possible, if any setup steps return an error. This also makes it possible to test a lot more.
Maybe use the urfave/cli package instead of flags?

I’ll add to the list if I come up with more.
Please say what you think.

Here’s some things I’d like to do, if you approve? - Replace as much as possible of package variables and init() functions with package ”constructors” and struct instances. This makes it a lot easier to test pieces in isolation, and also makes it possible to run tests in parallel without risk of race conditions. - Merge the top level context with signal handling, for cleaner / easier shutdown handling. This context should be passed to all goroutines, so they can exit cleanly on shutdown signals, and not leak. Also use this context during sleep, so one doesn’t have to wait for the sleep to finish, if sending a quit signal. - Split up large functions like main in smaller ones and return errors from those who need to instead of panicking all over the place, and then only have one top level panic possible, if any setup steps return an error. This also makes it possible to test a lot more. - Maybe use the urfave/cli package instead of flags? I’ll add to the list if I come up with more. Please say what you think.

nex commented

2026-01-16 02:58:40 +00:00

Owner

Replace as much as possible of package variables and init() functions with package ”constructors” and struct instances. This makes it a lot easier to test pieces in isolation, and also makes it possible to run tests in parallel without risk of race conditions.

I'd have to see it to believe in it, but I probably wouldn't be opposed. The whole init() and package var stuff is a hack to begin with, but was a cleaner hack than the alternatives at the time. Given #6 is putting the spotlight on making the command code in particular less ass (the technical definition), doing things more properly/idiomatically is absolutely a win regardless.

Merge the top level context with signal handling, for cleaner / easier shutdown handling. This context should be passed to all goroutines, so they can exit cleanly on shutdown signals, and not leak. Also use this context during sleep, so one doesn’t have to wait for the sleep to finish, if sending a quit signal.

The project was initially started before I knew what contexts were really used for, and that does evidently show. There's been multiple commits in recent history that have cleaned up context usage, but deadlocks (and probably leaks) are still a problem. Contributions to un-scuffling that is appreciated!

Split up large functions like main in smaller ones and return errors from those who need to instead of panicking all over the place, and then only have one top level panic possible, if any setup steps return an error. This also makes it possible to test a lot more.

Also something I've been working on recently. Historically I didn't pay much mind to the panics since I figured "well I just won't hold it wrong", but that isn't a rabbit hole worth going down (i.e. it gets to a point of "why even have errors at all"). In particular the panics make the database rather ill since it seems that it doesn't get to close properly sometimes (possibly also related to aforementioned context issues), so that needs fixing soon anyway.

Maybe use the urfave/cli package instead of flags?

Not too sure about this one, sensebot isn't really a CLI tool, the current flags do what they need to. The only real case I can see is for making ./sensebot -login less gross, but honestly that entire utility could be put on the chopping block in favour of mxtoken anyway.

> Replace as much as possible of package variables and init() functions with package ”constructors” and struct instances. This makes it a lot easier to test pieces in isolation, and also makes it possible to run tests in parallel without risk of race conditions. I'd have to see it to believe in it, but I probably wouldn't be opposed. The whole init() and package var stuff is a hack to begin with, but was a cleaner hack than the alternatives at the time. Given #6 is putting the spotlight on making the command code in particular less ass (the technical definition), doing things more properly/idiomatically is absolutely a win regardless. > Merge the top level context with signal handling, for cleaner / easier shutdown handling. This context should be passed to all goroutines, so they can exit cleanly on shutdown signals, and not leak. Also use this context during sleep, so one doesn’t have to wait for the sleep to finish, if sending a quit signal. The project was initially started before I knew what contexts were really used for, and that does evidently show. There's been multiple commits in recent history that have cleaned up context usage, but deadlocks (and probably leaks) are still a problem. Contributions to un-scuffling that is appreciated! > Split up large functions like main in smaller ones and return errors from those who need to instead of panicking all over the place, and then only have one top level panic possible, if any setup steps return an error. This also makes it possible to test a lot more. Also something I've been working on recently. Historically I didn't pay much mind to the panics since I figured "well I just won't hold it wrong", but that isn't a rabbit hole worth going down (i.e. it gets to a point of "why even have errors at all"). In particular the panics make the database rather ill since it seems that it doesn't get to close properly sometimes (possibly also related to aforementioned context issues), so that needs fixing soon anyway. > Maybe use the urfave/cli package instead of flags? Not too sure about this one, sensebot isn't really a CLI tool, the current flags do what they need to. The only real case I can see is for making `./sensebot -login` less gross, but honestly that entire utility could be put on the chopping block in favour of [mxtoken](https://timedout.uk/mxtoken.html) anyway.

oddlid commented

2026-01-16 09:25:22 +00:00

Author

Contributor

Alright! I'll go ahead then, but leave the flags along for now :)

nex referenced this issue from a pull request that will close it,

2026-01-23 15:34:41 +00:00

refactor #8

nex closed this issue

2026-02-27 13:19:31 +00:00

Rows
Columns

Refactor #7