Closed jinnatar closed 1 year ago
That all makes sense, thanks for the thorough review!
I think this might now be in a sane state, but was only able to test a couple of the notifiers myself. @martomi if you have some test farm for all the notifiers, could you give it a whirl and make sure I didn't miss anything?
I think I've fixed the ones where there was a clear fix. Happy to tidy up the default values if we have a consensus on which source of truth should be the new defaults. My feeling is that it's unlikely many folks have relied on the code defaults and instead have used the example values instead of explicitly deleting those fields from their config. But, alas I have no way of proving that feeling!
I've got the keepalive structural changes and a keepalive emitter for the wallet ready for review once we nail down this bit, just want to rebase them first on top of this one so you get a clean review slate.
Cool - thanks for the quick turnaround!
Note that there are a few more inline comments I left in the code above, but they’re collapsed in the middle of the thread by the github UI. Just wanna make sure you saw them too? In one of them I had provided more context and example for default values.
My take is that we should use the code defaults for the following reasons:
chiadog
before we introduced most of the configs in the example config. In the process of installing it, they would have created a copy of the very old config example. We never explicitly required them to change their original configs with the new values from the extended example config. Thus they’ll be having a config that is substantially more minimal than the current example. By keeping the defaults same as in the code, we’ll not introduce any unexpected changes for these users.Oops, not sure why "close" is on by default in review mode. But did not intend to close and now can't reopen :D
Hmm not sure, the reopen button is also grayed out for me. But more importantly, the code-diff is missing, so maybe that’s the main reason? Maybe you rebased on the wrong base? Feel free to re-open a new PR.
Yeah, looks like there's no sane way to re-open after a rebase & close. I'll open a new PR.
This is very much a work in progress, just wanted to be transparent and make the current status visible. The current state is able to run the example config and known minimal configs but needs more testing.
This change touches all integrations and has minor potential for subtle breakage. The following have been thoroughly tested:
Groundwork for fixing #361