-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor(thermal, renewable): create user classes #79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just non blocking minor comments
prepro: Optional[pd.DataFrame] = None, | ||
modulation: Optional[pd.DataFrame] = None, | ||
series: Optional[pd.DataFrame] = None, | ||
CO2Cost: Optional[pd.DataFrame] = None, | ||
fuelCost: Optional[pd.DataFrame] = None, | ||
co2_cost: Optional[pd.DataFrame] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, the case was ugly
from pydantic.alias_generators import to_camel | ||
|
||
|
||
class APIBaseModel(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, maybe we could move it to a base_model.py module for more visibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes why not it's a bit hidden inside the init file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do it inside another PR :)
return RenewableClusterPropertiesAPI.model_validate(user_dict) | ||
|
||
def to_user_model(self) -> RenewableClusterProperties: | ||
return RenewableClusterProperties( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks kind of dirty that it passes the type checking since strictly speaking the properties are optional now.
But we can dismiss that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. We have the same issue concerning the settings. What should we do ? Introduce the same object with the default fields ?
No description provided.