Closed truth-quark closed 3 years ago
I've dug further into the parameter file related code & have uncovered some code smells.
The [Slc|Dem]ParFileParser
classes are potentially redundant layers to simply return tuples of values.
A related problem: the parsers rely on the py_gamma.ParFile
class. This is an untested, 320 line partial reimplementation of Python’s dict
with a slightly different API. It looks like 1st draft code from a new Py developer as it reimplements features already available in Python core libs.
Can replace with a simpler class inheriting from dict
or mapping
& linking to I/O (i.e. handle the dict interface and I/O in one param file class). Having param files as dicts is simpler as it reuses Python’s dict API instead of creating a second interface (the case as of Nov 2020). The replacement class is probably best placed in the py_gamma_ga
module.
The core of this task (deduplication of classes) has been addressed in #221 , the duplicate classes have been deleted entirely as they were not needed. calc_baselines_new.py still has an SlcParFileParser which does quite a lot more than the smaller ones i deleted, and remains intact.
Addressing the open questions/tasks:
Can these be refactored into 2 classes to reduce tech debt/remove duplicates?
They could have, if they were even needed.
Are any of these dead code?
No, they were actively used (barely).
Is there import confusion across the project? (e.g. which one to use)
Yes, coregister*.py files used one copy, and calc_baselines_new.py had a very different implementation.
How much need is there for param file readers in the project?
Zero need in coregister*.py (someone got trigger happy with encapsulating things in objects I think), I've deleted them entirely and used py_gamma.ParFile directly instead (no extra lines of code required, that's how useless the wrappers were)
Inherit from https://docs.python.org/3/library/collections.abc.html#collections.abc.MutableMapping to add Pythonic dict like behaviour? This should remove some Gamma style get_value/set_value calls, removing the inefficiency of using a command line program to manipulate a key/value file...
No need, classes deleted.
If all param files have the same format keyword: value, will 2 classes suffice? ParFile & ReadOnlyParFile
It would if we were to ever create our own classes (for now I've kept using the py_gamma ones)
Implement at_exit() to auto save when closed (or use enter() context manager if possible?)
As above.
There are 3
SlcParFileParser
classes & 2DemParFileParser
classes:Open questions/tasks:
dict
like behaviour? This should remove someGamma
styleget_value
/set_value
calls, removing the inefficiency of using a command line program to manipulate a key/value file...keyword: value
, will 2 classes suffice?ParFile
&ReadOnlyParFile
at_exit()
to auto save when closed (or use__enter__()
context manager if possible?)Changes will affect 2 components of the project: