Adjust to EMGUI v0.6.0 - #21
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adapts EnergyModelsHeat to work with EnergyModelsGUI v0.6.0, which introduced breaking changes to the descriptive_names functionality. The PR restructures the descriptive names configuration to align with the new API requirements and adds comprehensive test coverage to ensure all TimeProfile fields have proper descriptions.
Key changes:
- Restructured
descriptive_names.ymlto nest type definitions underEnergyModelsHeatmodule - Added automated tests to verify descriptive names exist for all TimeProfile fields across different model types
- Added EnergyModelsGUI as a test dependency
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/test_descriptive_names.jl | Adds test suite to validate descriptive names coverage for types and variables across different heat models |
| test/Project.toml | Adds EnergyModelsGUI dependency for testing |
| ext/EMGUIExt/descriptive_names.yml | Reorganizes descriptive names under module namespace and updates field descriptions to align with v0.6.0 structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
See my comment regarding the Project.toml.
It would be also good to provide links to the breaking release of another package. If multiple people are reviewing a PR (not the case here), it saves time if the links are provided by the person creating the PR.
There was a problem hiding this comment.
There is a problem in the changes in the Project.toml: The way the weak dependency is introduced does not make sense. Without specifying an extension, there is not point to specify a weak dependency as outlined in the Pkg documentation.
As the EMGUI extension does not provide new code (only the YAML file), I would suggest removing the weak dependency. Note that I will not allow for EMGUI in the CI test set anyhow (as tested out in, e.g., 226361d) as loading the package takes too long.
There was a problem hiding this comment.
If deps for EMGUI is removed there will be an error with EMGUI v0.5* versions, and in this sense this would be a breaking change?
There was a problem hiding this comment.
This is a bit of a hen and egg problem, I see it now. That implies it requires a more thorough thought process how we handle it.
In theory, if we provide the file here, we need to have the version number regarding EMGUI to limit the application to the latest version to which it is compatible. In this case, we would see EMGUI as one of the core packages to which all other are dependent. The disadvantage is that if there are changes in the files in EMGUI, we would need to adjust all packages (as it is the case here right now)
The alternative is that all names are included in EMGUI for the supported packages of the EnergyModelsX organization. In this situation, we unfortunately would require changes in EMGUI for changes in the individual packages which is neither beneficial. We would also require a dependency for each package to limit its applicability to a given version.
I would still argue to remove the compatibility. We did not have a limitation beforehand for v0.5. As a consequence, it is possible to use 0.1.3 (the version before this) with EMGUI v0.6.0. If people update their package versions with pgk> update, they will anyhow update to the latest version of both. In the second case of having either of the package cloned or downloaded, the compatibility in EMHeat does not change the picture.
This PR enables
EnergyModelsHeatusage with EnergyModelsGUI (EMGUI) version 0.6.0 (that introduced breaking changes fordescriptive_names).The PR attempted to also introduce test to verify that all variables/fields in the module being of type
TimeProfilehas a description. That is, if new structs or variables are introduces withTimeProfiles, one must updateext/EMGUIExt/descriptive_names.ymlcorrespondingly (but not that this is not necessary if the field can be inherited from super types). However, this seem not to be possible as Makie (an EMGUI dependency) does not support CI for windows. Locally, one could run the following tests instead: