Prohibit the use of / in component names#561
Conversation
Adds docstrings for exported types and functions that were missing them: AbstractDeterministic, FunctionData, ProductionVariableCostCurve, CompressionTypes, NormalizationTypes, UnitSystem, get_num_components, and make_logging_config_file. Uses @doc macro for @scoped_enum types since they expand to module definitions that can't use standard docstrings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add docstrings to all exported symbols
Change units_info field type from Union{Nothing, UnitsData} (abstract) to
Union{Nothing, SystemUnitsSettings} (concrete). This allows the compiler
to infer the type of units_info.base_value after an isnothing check,
eliminating dynamic dispatch in hot paths like get_value/set_value.
Benchmark impact (PSY ACTIVSg10k, 15k component get/set loop):
Before: 4.0 ms (174k allocations)
After: 552 μs (19k allocations) — 7.3x faster
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use concrete type for units_info field
There was a problem hiding this comment.
Pull request overview
Adds validation to prohibit / in component names (to avoid HDF5 dataset path issues), with an escape hatch via an environment variable.
Changes:
- Introduces
_validate_component_nameand enforces it when adding components and renaming components. - Adds tests asserting
/is rejected by default and accepted whenSIENNA_DISABLE_COMPONENT_NAME_CHECKS=true.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/components.jl |
Adds component-name validation and applies it in _add_component! and set_name!. |
test/test_system_data.jl |
Adds test coverage for invalid / in names and env-var bypass behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #561 +/- ##
==========================================
+ Coverage 77.81% 78.94% +1.13%
==========================================
Files 75 75
Lines 6680 6597 -83
==========================================
+ Hits 5198 5208 +10
+ Misses 1482 1389 -93
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
daniel-thom
force-pushed
the
psy-issue-1647
branch
from
0c4bde7 to
23bd93a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| withenv("SIENNA_DISABLE_COMPONENT_NAME_CHECKS" => "true") do | ||
| data2 = IS.SystemData() | ||
| slash_component = IS.TestComponent("E/W", 5) | ||
| IS.add_component!(data2, slash_component) | ||
| @test IS.get_name(slash_component) == "E/W" |
There was a problem hiding this comment.
The test mutates the global ENV and then unconditionally deletes the key in finally, which will drop any pre-existing value and can leak state to subsequent tests in the same process. Prefer Base.withenv("SIENNA_DISABLE_COMPONENT_NAME_CHECKS" => "true") do ... end (or save/restore the previous value) so the original environment is reliably restored.
daniel-thom
force-pushed
the
psy-issue-1647
branch
from
5afe43f to
83f3820
Compare
|
@daniel-thom to avoid breaking users during the current version I suggest we do this in IS4. |
| # Refer to https://github.com/NREL-Sienna/PowerSystems.jl/issues/1647 for the trigger of | ||
| # this rule. The '/' character could also cause problems if we ever wanted to use the | ||
| # component name as a file or directory name. | ||
| const INVALID_COMPONENT_NAME_CHARACTERS = ('/',) |
There was a problem hiding this comment.
@josephmckinsey we need to enforce this somewhere in the DB and Schema for the existing versions that's fine but in the future IS4 and PSY6 we should eliminate it
|
@daniel-thom we can close this and handle it in IS4 later since it is a corner case |
No description provided.