Refactor to avoid the use of a special global object. #544
Conversation
The global object formerly used is now replaced by direct use of the namespace opf the globals module. This eliminates the redundant getters and setters and simplifies the code for future maintainers. Note that the globals module name conflicts (harmlessly at present) with a Python built-in function. A future commit should rename it `config` to remove this clash and better represent its intended purpose.
|
|
|
Ah, I see I borked the code. For some reason I find pylint unusable locally - it finds far more errors than the CI chain is doing. I'll fix the errors I see reported in the CI output and amend the PR. Sorry about the hassle. |
Also refactor to silence some CI issues.
Since one of pylint's complains was that the globals module was shadowing the built-in, and since the name `config` was already is use in several modules, globals.py was renamed as mt_config.py. All tests now pass, and the only remaining local pylint errors relate to the protobuf code, I'm hoping this will make the PR valid.
|
Will try to take a look at this later, but I'll at least set off a CI run in case there's any other issues there that might come up. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #544 +/- ##
==========================================
- Coverage 63.77% 63.20% -0.57%
==========================================
Files 14 14
Lines 2857 2813 -44
==========================================
- Hits 1822 1778 -44
Misses 1035 1035
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I see that the test coverage going down has raised a flag. I suspect this may be because I deleted a couple of tests that became redundant with the removal of a global object. I trust this slight negative is outweighed by the positive impact of the changes. The code base has actually shrunk by almost 100 lines. |
|
Yeah, I wouldn't block it on the basis of just that. There's a lot of improvements to be made to the tests, heh. |
|
I've been looking for an interesting Python radio-oriented project to get my teeth into. Although I was originally licensed in 1965 (!) my radio skills are nowadays pretty rudimentary, but I have a lot of Python experience and would love to help. |
|
Sorry that it took me so long to get to reviewing this properly. Obviously this is a small change and it'll be nice if we can move away from needing anything to do with globals outright eventually, but it does seem a bit cleaner for the time being, so seems like a good thing to include. Thanks for the pull request! |
|
Thanks for merging. I'm currently looking at a whole range of potential refactorings, but before submitting further pull requests would appreciate the ability to talk about them first, if you have chance. I don't want to start proposing changes that don't align with your future direction. |
Yes, ideally each interface would be completely self-contained and allow us to build sensible systems with multiple radios. Regard this as a first simplification step along the way. |
|
I'm happy to chat about stuff, for sure! I don't know if you're on the Meshtastic discord but I hang around there a decent amount (I'm |
3 participants
The global object formerly used as a singleton is now replaced by direct use of the namespace of the globals module. This eliminates the redundant getters and setters, and both clarifies and simplifies the code for future maintainers.
Note that the globals module name conflicts (harmlessly at present) with a Python built-in function. A future commit could rename it
configto remove this clash and better represent its intended purpose.The impact of this change may suggest other fairly simple improvements of a similar nature to improve maintainability and testability.