Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor RuntimeConfig #1173

Merged
merged 9 commits into from
Feb 17, 2025
Merged

Refactor RuntimeConfig #1173

merged 9 commits into from
Feb 17, 2025

Conversation

staudtMarius
Copy link
Member

Resolves #1172

@staudtMarius staudtMarius added code quality Code readability or efficiency is improved config Issues related to the configuration of simulation runs labels Feb 12, 2025
@staudtMarius staudtMarius self-assigned this Feb 12, 2025
@staudtMarius
Adding scaladoc.
3d85d8b
@staudtMarius
Merge branch 'dev' into ms/#1172-refactor-runtime-config
eb2b5ef 
# Conflicts:
#	gradle/scripts/documentation.gradle
#	src/main/scala/edu/ie3/simona/config/SimonaConfig.scala
@staudtMarius
Adapting to changes in dev.
94099dc
@staudtMarius staudtMarius requested review from danielfeismann and sebastian-peter and removed request for danielfeismann February 14, 2025 08:56
sebastian-peter
sebastian-peter requested changes Feb 14, 2025
View reviewed changes
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check out the SonarQube issues

@staudtMarius staudtMarius mentioned this pull request Feb 14, 2025
Closed
sebastian-peter
sebastian-peter requested changes Feb 14, 2025
View reviewed changes
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adapt parameter names in vn_simona.conf as well.

Also, config-template.conf is not required anymore. For me it'd be fine if you just deleted it in this PR, but separate PR is also fine.

sebastian-peter
sebastian-peter reviewed Feb 14, 2025
View reviewed changes
src/main/scala/edu/ie3/simona/config/ArgsParser.scala
@@ -225,8 +225,8 @@ object ArgsParser extends LazyLogging {
ConfigFactory.parseString(
s"""config = "${parsedArgs.configLocation.get.replace("\\", "\\\\")}"
|simona.runtime_configuration {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is supposed to be simona.runtime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I checked this part and it was simona.runtime_configuration in the initial commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know, if we actually need this part? It is given to the ActorSystem, but I don't see any place it is ever used.

Copy link
Member

@sebastian-peter sebastian-peter Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just throw out selectedSubgrids and selectedVoltLvls from ArgsParser, now that I think about it. I think we never really used it, and if one wants to use it, it can still be configured through the conf file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then also, there's lots of stuff in ArgsParser that can be thrown out. So maybe we should create a separate issue for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1192 -> This PR can just stay as it is in that regard

@staudtMarius
Copy link
Member Author

staudtMarius commented Feb 17, 2025 •
edited
Loading

Please adapt parameter names in vn_simona.conf as well.

Also, config-template.conf is not required anymore. For me it'd be fine if you just deleted it in this PR, but separate PR is also fine.

I deleted it in this PR.

staudtMarius and others added 2 commits February 17, 2025 10:43
@staudtMarius
Merge branch 'dev' into ms/#1172-refactor-runtime-config
e245f78 
# Conflicts:
#	src/main/resources/config/config-template.conf
#	src/main/scala/edu/ie3/simona/config/SimonaConfig.scala
@sebastian-peter
Merge branch 'dev' into ms/#1172-refactor-runtime-config
8fa96f2 
# Conflicts:
#	CHANGELOG.md
sebastian-peter
sebastian-peter approved these changes Feb 17, 2025
View reviewed changes
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the effort!

@sebastian-peter sebastian-peter merged commit f044120 into dev Feb 17, 2025
4 checks passed
@sebastian-peter sebastian-peter deleted the ms/#1172-refactor-runtime-config branch February 17, 2025 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastian-peter sebastian-peter sebastian-peter approved these changes

@danielfeismann danielfeismann Awaiting requested review from danielfeismann

Assignees

@staudtMarius staudtMarius

Labels
code quality Code readability or efficiency is improved config Issues related to the configuration of simulation runs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor RuntimeConfig
2 participants
@staudtMarius @sebastian-peter