I am looking for a code review on the following code. Say I have a simple example class, something like the following:
from dataclasses import dataclass
@dataclass
class MyClass:
attribute1: int
attribute2: int
attribute3: int
attribute4: int
and a config.ini file that contains the following:
[DefaultInstance1]
attribute1=1
attribute2=2
attribute3=3
attribute4=4
[DefaultInstance2]
attribute1=1
attribute2=4
attribute3=9
attribute4=16
I want to be able to simply add Defaults to my config and make them directly available to the user. After a lot of thinking I came up with the following pattern:
import configparser
from types import SimpleNamespace
def create_instances_from_config(config_file):
config = configparser.ConfigParser()
config.read(config_file)
instances = {}
for section in config.sections():
if section.startswith('DefaultInstance'):
options = config[section]
instance = CustomClass(
options.get('attribute1'),
options.get('attribute2'),
options.get('attribute3'),
options.get('attribute4')
)
instances[section] = instance
return SimpleNamespace(**instances)
preconfigured = create_instances_from_config(config)
Now I can import 'preconfigured' in any other file, and pass any preconfigured CustomClass instance around with easy namespacing, something like: preconfigured.DefaultInstance1. It looks pretty clean to me, however, I was wondering if there were any other thoughts about this approach. Are there other, maybe more simple, or less error prone ways to do this? Am I overcomplicating things? Also, I would love to read more about what I am trying to do here, are there specific names for these type of operations? Any feedback or thoughts would be very much appreciated! Thanks! :)
2 Answers 2
I like it! It looks good.
finding sections
for section in config.sections():
if section.startswith('DefaultInstance'):
If you need to do such prefix matching, then fine, this works great, with linear cost to scan the whole .ini file.
For the sequentially numbered sections in your example,
it would make more sense to probe them directly.
Iterate over positive integers, and ask
whether section[f"DefaultInstance{i}"]
exists,
perhaps using .get()
.
informative identifier
create_instances_from_config
does not exactly seem
like a terrific name for your Public API.
You have not revealed the use case, the business motivation for this code.
Hopefully it offers some vocabulary words that are more
informative than "instance", which seems rather dry and synthetic.
Similarly, I'm not crazy about preconfigured
.
defaults
, maybe?
parameters
Your example code might have passed in "DefaultInstance"
as an input argument -- no biggie.
Similarly for attributes 1 .. 4. But this is sort of an XY problem. Your actual code of interest might need such hardcodes.
Recall that glom can help with accessing nested dicts, if that's the underlying pain point you were trying to address.
Is Something Missing?
It seems to me something is missing from your code. You say in part:
Now I can import 'preconfigured' in any other file, and pass any preconfigured CustomClass instance around ...
I am focusing on the phrase "and pass any preconfigured CustomClass instance around" with emphasis on the word any. Yet your code instantiates an instance of of a specific class MyClass
, which is not defined. In your introduction to your solution you presented class MyClass
, which I assume would be an example of such an any class that you might want to instantiate. If this is so, then your function create_instances_from_config
should take as an additional argument the class you wish to instantiate. For example,
import configparser
from types import SimpleNamespace
def create_instances_from_config(config_file, cls):
config = configparser.ConfigParser()
config.read(config_file)
instances = {}
for section in config.sections():
if section.startswith('DefaultInstance'):
options = config[section]
instance = cls(
int(options.get('attribute1')),
int(options.get('attribute2')),
int(options.get('attribute3')),
int(options.get('attribute4'))
)
instances[section] = instance
return SimpleNamespace(**instances)
if __name__ == '__main__':
from dataclasses import dataclass
@dataclass
class MyClass:
attribute1: int
attribute2: int
attribute3: int
attribute4: int
preconfigured = create_instances_from_config('test.ini', MyClass)
print(preconfigured.DefaultInstance1)
Prints:
MyClass(attribute1=1, attribute2=2, attribute3=3, attribute4=4)
Or have I missed something in your code? Also note that your class MyClass
defines its attributes as being of type int
, but option values from a config file are strings and therefore they must be converted before instantitaing MyClass
.
Additional Suggestions
It's a small matter, but your argument name to create_instances_from_config
is named config_file. I tend to think of a file as what is returned from the built-in open
function. Perhaps a better name for this argument might be config_path. In any event, using typing or a docstring would make this clearer:
def create_instances_from_config(config_path: str, cls: type) -> SimpleNamespace:
...
Adding an additional docstring describing what the function does would be a plus.
A More Generic Version
This may not be at all relevant to your situation, but if you needed a more generic version that did not assume anything about the actual option names and their value types, then you could return a dictionary of dictionaries. I would male create_dict_from_config
a method of class DefaultConfig
that would allow you to create subclasses that override this method to convert the option values to the required types. For example:
import configparser
class DefaultConfig:
def create_dict_from_config(self, config_path: str) -> dict:
"""Reads in a configuration creating a dictionary whose
keys are the section names that begin with 'DefaultInstance'
and whose values are dictionaries of option-name: option-value pairs.
For example, if the configuration file is ...
[DefaultInstance1]
attribute1=1
attribute2=2
attribute3=3
attribute4=4
[DefaultInstance2]
attribute1=1
attribute2=4
attribute3=9
attribute4=16
[Some Other Sections]
...
... then this function returns the following dictionary:
{
'DefaultInstance1':
{'attribute1': '1', 'attribute2': '2', 'attribute2': '3', 'attribute2': '4'},
'DefaultInstance2':
{'attribute1': '1', 'attribute2': '4', 'attribute2': '9', 'attribute2': '16'},
}
"""
config = configparser.ConfigParser()
config.read(config_path)
sections = {}
for section in config.sections():
if section.startswith('DefaultInstance'):
sections[section] = dict(config[section])
return sections
if __name__ == '__main__':
from dataclasses import dataclass
@dataclass
class MyClass:
attribute1: int
attribute2: int
attribute3: int
attribute4: int
class MyDefaultConfig(DefaultConfig):
def create_dict_from_config(self, config_path: str) -> dict:
"""Coverts the option values to int types."""
sections = DefaultConfig.create_dict_from_config(self, config_path)
for section_name, section_dict in sections.items():
sections[section_name] = {k: int(v) for k, v in section_dict.items()}
return sections
my_default_config = MyDefaultConfig()
preconfigured = my_default_config.create_dict_from_config('test.ini')
default_config_1 = MyClass(**preconfigured['DefaultInstance1'])
print(default_config_1)
Prints:
MyClass(attribute1=1, attribute2=2, attribute3=3, attribute4=4)