-
Notifications
You must be signed in to change notification settings - Fork 81
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
Document/fix surprising behaviors of set_name!(sys, component, name)
#1161
Comments
set_name!(sys, ...)
changes the container, not just the componentset_name!(sys, component, name)
changes the container, not just the component
set_name!(sys, component, name)
changes the container, not just the componentset_name!(sys, component, name)
I don't think we can do much here, besides adding in the documentation that every time you modify a component you should do:
Regarding the second issue of renaming to same name, @GabrielKS can you open an issue on InfrastructureSystems? Regarding the main issue here: |
@rodrigomha yes, it is on my general to-do's to add a "manipulating data" tutorial to highlight the getters and setters, some of which was previously on other pages and is now orphaned. I'll take a note of this. |
@rodrigomha Opened NREL-Sienna/InfrastructureSystems.jl#397 for the second issue. |
@annacasavant See this whole thread, especially Rodrigo's note |
If one renames components attached to a system in a loop over
get_components
:some components will be covered multiple times:
because the components are stored in a dictionary indexed by name. It's reasonable to expect people to know that when iterating over a container, they're not supposed to change the container (so no adding and removing components in this loop), but I don't think it's obvious to users that
set_name!
changes the container, not just a field within the component (like, if this were something likeset_peak_active_power!
, it would be safe). @daniel-thom and I agree that it's not at all worth changing the implementation to make this behavior safe, but that we should document more explicitly thatset_name!
changes the container.The text was updated successfully, but these errors were encountered: