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.

Sign up for GitHub

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

Jump to bottom

Should all settings classes inherit from Pydantic BaseSettings via HermesSettings? #302

Open
sdruskat opened this issue Jan 9, 2025 · 3 comments · May be fixed by #303
Open

Should all settings classes inherit from Pydantic BaseSettings via HermesSettings? #302

sdruskat opened this issue Jan 9, 2025 · 3 comments · May be fixed by #303
Labels
architecture Describes some architectural decisions that need to be made question Further information is requested
Milestone
0.9.0

Comments

@sdruskat
Copy link
Contributor

sdruskat commented Jan 9, 2025

Currently, the (some?) settings classes for plugin implementations (e.g. for the CFF harvester) inherit from pydantic.BaseModel. I believe that this is not correct (see #301).

Furthermore, currently hermes.commands.base.HermesSettings inherits from pydantic_settings.BaseSettings. This is clever because BaseSettings allows interaction with env vars. (Also, BaseSettings inherits from BaseModel.)

However, e.g., hermes.commands.harvest.base.HarvestSettings again directly inherits from BaseModel.

Q: Shouldn't we redefine the base settings classes for all commands to inherit from BaseSettings rather than from BaseModel?
@sdruskat sdruskat added question Further information is requested architecture Describes some architectural decisions that need to be made labels Jan 9, 2025
@sdruskat
Copy link
Contributor Author

sdruskat commented Jan 9, 2025

More concretely, this is what things look like now:

grafik

I think they should look like this:

grafik

@sdruskat
Copy link
Contributor Author

sdruskat commented Jan 9, 2025

Okay, after some discussion with @SKernchen we concluded that, no, the settings implementation classes (here: NewHarvestSettings) should inherit from BaseModel to disallow access to the properties of other classes.

If this is the case - and @led02 can confirm - I think this means that we should set visibility of HarvestSettings and HermesSettings down (i.e., make them "private").

@sdruskat
Copy link
Contributor Author

sdruskat commented Jan 9, 2025

Decided with @led02 that the best way forward is indeed to make these two classes private.

This was referenced Jan 10, 2025
Closed
Open
@sdruskat sdruskat added this to the 0.9.0 milestone Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
architecture Describes some architectural decisions that need to be made question Further information is requested
Projects
None yet
Milestone
0.9.0
Development

Successfully merging a pull request may close this issue.

Minor architectural fixes softwarepub/hermes
1 participant
@sdruskat