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
No description provided.