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.

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

refactor: configuration improvements phase 1 #1488

Merged
merged 36 commits into from
Jul 7, 2023
Merged

Conversation

takb
Copy link
Contributor

@takb takb commented Jul 7, 2023

Information about the changes

  • configuration through application.yml, overridden by application-test.yml/application-unittest.yml for tests and ors-config.yml for actual use
  • Properties classes to read said configs
  • EngineConfig class to transport what is needed into the engine module
  • All previous json config parameters are considered and take precedence
  • Moved some API parameters to correct places
  • Improved logging with more consistent setup
  • Removed several unused classes

This is a WIP merge, code compiles and runs and all tests succeed, but the new configuration method is not completely functional, especially the profiles configuation part.

takb and others added 30 commits June 21, 2023 16:07
The usage of the static MatrixServiceSettings has been moved towards the
API-boundary as a preparation to the new configuration system. As soon
as the new config system is in place at the APi-side, the remaining
static usages of MAtrixServiceSettings can be replaced.
The following matrix config parameters of the new configuration system have
been integrated: maximum_visited_nodes, maximum_search_radius.
The new config system should replace all usages of the static *ServiceSettings
classes. This commit reduces the number of usages of MatrixServiceSettings.
The remaining usages require larger refactorings ans shall be addressed
separately.
This commit removes the usage of the static MatrixServiceSettings
from AbstractManyToManyMatrixAlgorithm in favor of the new
configuration system. Unfortunately, the current structure of
the matrix algorithms forced this commit to pass around the
boolean "hasInfiniteUTurnCosts" through several classes. This
should be cleaned up together with the matrix algorithms in
a later commit.
The static *ServiceSettings classes are to be removed in favor of
the new configuration system. This commit reduces the number of
usages of RoutingServiceSettings.
@takb takb changed the title Refactor: engine config phase 1 refactor: engine config phase 1 Jul 7, 2023
@takb takb changed the title refactor: engine config phase 1 feat: config refactoring phase 1 Jul 7, 2023
@github-actions github-actions bot added feature and removed refactor labels Jul 7, 2023
@takb takb requested a review from MichaelsJP July 7, 2023 10:07
@github-actions github-actions bot added feature and removed feature labels Jul 7, 2023
@takb takb marked this pull request as ready for review July 7, 2023 10:23
@github-actions github-actions bot added feature and removed feature labels Jul 7, 2023
@takb takb changed the title feat: config refactoring phase 1 feat: configuration improvements phase 1 Jul 7, 2023
@github-actions github-actions bot added feature and removed feature labels Jul 7, 2023
@MichaelsJP MichaelsJP changed the title feat: configuration improvements phase 1 refactor: configuration improvements phase 1 Jul 7, 2023
@github-actions github-actions bot added refactor and removed feature labels Jul 7, 2023
@MichaelsJP
Copy link
Member

MichaelsJP commented Jul 7, 2023

Imo “refactor” fits it best right now. When the new config split will be available as the main entrance to the user, I'd say we call that switch "feature".

@takb takb enabled auto-merge July 7, 2023 11:10
@takb takb merged commit 39203fc into master Jul 7, 2023
@takb takb deleted the refactor/engine-config branch July 7, 2023 13:07
@MichaelsJP MichaelsJP mentioned this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants