-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add new type ComponentId
#795
base: v1.x.x
Are you sure you want to change the base?
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.
I would really avoid using NewType
in this case, also I'm not sure if we should update all component_id
to component
as how the component is specified now is encoded in the type system, and could open the door to pass a component as an object for example (or even think more on a case-by-case basis if another name is more suitable). But I guess that part is out of scope for this PR.
@@ -17,6 +17,9 @@ | |||
from ...timeseries import Fuse | |||
|
|||
|
|||
ComponentId = NewType("ComponentId", int) |
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.
Why NewType
and not dataclass
?
NewType
allows this for example: ComponentId(1) + 2
and microgrid.battery_pool(priority=ComponentId(1))
which is just weird.
Also the str
and repr
looks just like a bare int
, which might be good or not (I personally would like to at least have repr
use ComponentId(1)
instead of just 1
.
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.
Honestly, no specific reason, I selected NewType
after a quick research, I am not set on using it :)
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.
In that case I would use a dataclass
, I see no benefits in using NewType
and a few drawbacks.
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.
just a data class with one member you mean?
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.
Yup.
This is a WIP and the current work is 90% done by ChatGPT.
I am currently busy with a few other things, so anyone can take over this PR and finish it.
Otherwise I'll get back to it when my important stuff is done.