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

Use SteveConfiguration as Spring component #1202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

juherr
Copy link
Contributor

@juherr juherr commented Jul 29, 2023

Goal: Avoid global variable and inject instance everywhere needed.

Extra:

  • Extract UnknownChargePointService in order to remove the dependency cycle.
  • Prefer constructor injection instead of field injection when a class was modified

@juherr
Copy link
Contributor Author

juherr commented Aug 9, 2023

Ping @goekay wdyt?

@juherr
Copy link
Contributor Author

juherr commented Aug 20, 2023

Ping @goekay

@goekay
Copy link
Member

goekay commented Aug 29, 2023

this is hard for me to review as there are too many changes in this PR, but also in one commit. can you split it to multiple PRs, or at least multiple commits?

regarding the constructor injection: i would welcome it in conjunction with Lombok to reduce constructor ceremony using @RequiredArgsConstructor. moreover, i see you did not make the changes in all classes. may i ask the reason?

thanks for the effort!

@juherr
Copy link
Contributor Author

juherr commented Aug 29, 2023

moreover, i see you did not make the changes in all classes. may i ask the reason?

Because it was not the purpose of the pull request.
I've just modified classes that were impacted by the change.

As asked: #1226

@juherr
Copy link
Contributor Author

juherr commented Aug 29, 2023

this is hard for me to review as there are too many changes in this PR

Even if I rebase onto #1226, I'm not sure the change will be really shorter because the steve configuration global variable is used deep in the object graph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants