2
\$\begingroup\$

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! :)

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.