Refactor `BMConfigParser` as a module variable #1927

shekhar-cis · 2022-01-28T13:05:02Z

Refactor BMConfigParser

now as a module variable instead of singleton
can run unit tests
can be used in mock more easily

src/bmconfigparser.py

src/tests/test_config.py

src/helper_startup.py

src/inventory.py

src/class_sqlThread.py

src/bmconfigparser.py

g1itch · 2022-02-03T16:17:34Z

src/bmconfigparser.py

@@ -270,3 +169,6 @@ def validate_bitmessagesettings_maxoutboundconnections(value):
        if value < 0 or value > 8:
            return False
        return True
+
+
+config = BMConfigParser()


Another place with side effects is bad

What alternative design do you propose? I think a module variable is an improvement singleton and I don't know other approaches.

For me this approach seems worse than the singleton antipattern. Create the instance of BMConfigParser() upon program start and use it everywhere passing as argument to functions or accessing it through a slot of another object.

These changes also create potential conflicts all across the codebase. And current test coverage is definitely not enough for such big changes.

but where to put it then? state.py?

So you basically recommend dependency injection. Which is ok, I'm just not sure that can be done easily now because of legacy structures. Using a module variable is halfway there, we can do unit tests on isolated objects, and can be refactored into dependency injection later.

If it was all about the tests, you could just separate BMConfigParser from singleton object.

It's not just tests though, there are some additions that prevent misbehaviour, like a clean reset, consolidation of default values and threads waiting for initialization. I'm not super happy that the commit is kind of big, just the old code is difficult to modify in small chunks. The commit makes the code easier to change in the future.

src/default.ini

src/bmconfigparser.py

src/class_singleWorker.py

src/class_sqlThread.py

src/helper_startup.py

src/default.ini

src/bmconfigparser.py

src/bitmessageqt/settings.py

src/default.ini

src/tests/test_config.py

packages/pyinstaller/bitmessagemain.spec

src/tests/test_config.py

packages/pyinstaller/bitmessagemain.spec

PeterSurda · 2022-02-15T10:10:25Z

@g1itch I'm ready to merge this, any objections other than dependency injection? That can be done later and easier now with this refactoring.

g1itch · 2022-02-15T15:00:39Z

@g1itch I'm ready to merge this, any objections other than dependency injection? That can be done later and easier now with this refactoring.

No. Maybe I should accept the death of #1389.

PeterSurda · 2022-02-15T15:06:15Z

No. Maybe I should accept the death of #1389.

There's work being done on the mock backend and more flexible buildbot jobs. Once they are available, UI tests can be executed not only for kivy but also for Qt. Then #1389 can be tested more reliably and merged.

g1itch · 2022-02-16T20:10:15Z

It looks like SettingsDialog.net_restart_needed in the bitmessageqt is ignored in the appimage built with this changeset.

shekhar-cis force-pushed the refactor/bmconfigparser branch 7 times, most recently from 8863e48 to de05577 Compare February 1, 2022 15:04

PeterSurda requested changes Feb 2, 2022

View reviewed changes

shekhar-cis force-pushed the refactor/bmconfigparser branch 4 times, most recently from 6b1e897 to 04f4271 Compare February 2, 2022 16:05

PeterSurda reviewed Feb 3, 2022

View reviewed changes

src/class_sqlThread.py Outdated Show resolved Hide resolved

PeterSurda reviewed Feb 3, 2022

View reviewed changes

src/bmconfigparser.py Outdated Show resolved Hide resolved

shekhar-cis force-pushed the refactor/bmconfigparser branch 3 times, most recently from c649d1d to db39e6e Compare February 3, 2022 14:57

g1itch reviewed Feb 3, 2022

View reviewed changes