Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConfigBuilder allows Config class names with text following the "Config" portion of the class name #34

Open
aholmes opened this issue Dec 7, 2023 · 0 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers priority:3 Low priority issue

Comments

@aholmes
Copy link
Member

aholmes commented Dec 7, 2023

The convention with BL_Python's ConfigBuilder is that any config classes must end with Config, however, the way this is enforced allows for any text following the ending Config. This is problematic mainly because the portion of the config name preceding Config is used as the attribute name in the generated config type, and is also the expected key for a loaded TOML file (when using load_config). As such, this may lead to surprising or confusing behavior if someone were to use a class name with text following Config in a class name.

Working reproduction:

import BL_Python.programming.config as config
from pydantic import BaseModel

class Config(BaseModel, config.AbstractConfig):
    pass

class MyFooConfigExtra(BaseModel):
    # `my_foo` should end up as `Foo` by default
    my_foo: str = "Foo"

config_type = config.ConfigBuilder()\
    .with_root_config(Config)\
    .with_configs([MyFooConfigExtra])\
    .build()

loaded_config = config.load_config(config_type, 'config.toml')

print(loaded_config)

Working config.
Script prints myfoo=MyFooConfigExtra(my_foo='Bar')

[myfoo]
# `myfoo` gets set to the config value `Bar`, which is correct.
my_foo = 'Bar'

Example of possibly confusing config. This does not work.
Same result when using the key myfooconfigextra.
Script prints myfoo=None

[myfooextra]
# `myfoo` is not set to the config value `Bar` and ends up as `None`. This is not correct.
my_foo = 'Bar'

Note that in this failure case, it would not be expected for the ConfigBuilder to find this section because there is no MyFooExtraConfig passed to with_configs(...). So, in that sense, myfoo being None is "correct." The issue is that this is confusing to users of ConfigBuilder because the word "extra" is not correct to use, but seems like it should be used.

@aholmes aholmes added bug Something isn't working good first issue Good for newcomers labels Dec 7, 2023
@kiarod kiarod self-assigned this Dec 8, 2023
@aholmes aholmes added the priority:3 Low priority issue label Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers priority:3 Low priority issue
Projects
None yet
Development

No branches or pull requests

2 participants