You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by "a7med3del1973 (via GitHub)" <gi...@apache.org> on 2025/02/11 17:34:14 UTC
[PR] FINERACT-2081refactor-constructor-injection [fineract]
a7med3del1973 opened a new pull request, #4322:
URL: https://github.com/apache/fineract/pull/4322
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #4322:
URL: https://github.com/apache/fineract/pull/4322#issuecomment-2678243578
> Thanks for the feedback @adamsaghy ! I understand the need to increase the scope of PRs to maintain a cleaner commit history.
>
> For this PR, I initially focused on refactoring `AcmeNoopJobConfiguration` to ensure the changes were isolated and easy to review. However, I agree that a broader refactor across multiple modules would be more efficient.
>
> Would you prefer I extend this PR to cover additional modules, or should we merge this first and then follow up with a larger refactor in a separate PR? Let me know how you'd like to proceed.
Its okay to do it in the future ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "a7med3del1973 (via GitHub)" <gi...@apache.org>.
a7med3del1973 commented on code in PR #4322:
URL: https://github.com/apache/fineract/pull/4322#discussion_r1958684878
##########
custom/acme/loan/job/src/main/java/com/acme/fineract/loan/job/AcmeNoopJobConfiguration.java:
##########
@@ -24,20 +24,23 @@
import org.springframework.batch.core.launch.support.RunIdIncrementer;
import org.springframework.batch.core.repository.JobRepository;
import org.springframework.batch.core.step.builder.StepBuilder;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.transaction.PlatformTransactionManager;
@Configuration
public class AcmeNoopJobConfiguration {
- @Autowired
- private JobRepository jobRepository;
- @Autowired
- private PlatformTransactionManager transactionManager;
- @Autowired
- private AcmeNoopJobTasklet tasklet;
+ private final JobRepository jobRepository;
+ private final PlatformTransactionManager transactionManager;
+ private final AcmeNoopJobTasklet tasklet;
+
+ public AcmeNoopJobConfiguration(JobRepository jobRepository, PlatformTransactionManager transactionManager,
Review Comment:
@galovics
Changes have been made .. waiting your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "galovics (via GitHub)" <gi...@apache.org>.
galovics commented on code in PR #4322:
URL: https://github.com/apache/fineract/pull/4322#discussion_r1952773948
##########
custom/acme/loan/job/src/main/java/com/acme/fineract/loan/job/AcmeNoopJobConfiguration.java:
##########
@@ -24,20 +24,23 @@
import org.springframework.batch.core.launch.support.RunIdIncrementer;
import org.springframework.batch.core.repository.JobRepository;
import org.springframework.batch.core.step.builder.StepBuilder;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.transaction.PlatformTransactionManager;
@Configuration
public class AcmeNoopJobConfiguration {
- @Autowired
- private JobRepository jobRepository;
- @Autowired
- private PlatformTransactionManager transactionManager;
- @Autowired
- private AcmeNoopJobTasklet tasklet;
+ private final JobRepository jobRepository;
+ private final PlatformTransactionManager transactionManager;
+ private final AcmeNoopJobTasklet tasklet;
+
+ public AcmeNoopJobConfiguration(JobRepository jobRepository, PlatformTransactionManager transactionManager,
Review Comment:
You can use Lombok's `@RequiredArgsConstructor` instead of actually writing the code for the constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy merged PR #4322:
URL: https://github.com/apache/fineract/pull/4322
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "adamsaghy (via GitHub)" <gi...@apache.org>.
adamsaghy commented on PR #4322:
URL: https://github.com/apache/fineract/pull/4322#issuecomment-2677910578
@a7med3del1973 It looks okay, but it would be advisable to increase the scope of these PRs. For example here, you were refactoring `AcmeNoopJobConfiguration` to use constructor injection. You might wanna go through the whole Fineract (or at least one or multiple modules and do the same), we dont wanna have a separate commits for each classes...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "a7med3del1973 (via GitHub)" <gi...@apache.org>.
a7med3del1973 commented on code in PR #4322:
URL: https://github.com/apache/fineract/pull/4322#discussion_r1966528730
##########
custom/acme/loan/job/src/main/java/com/acme/fineract/loan/job/AcmeNoopJobConfiguration.java:
##########
@@ -24,20 +24,23 @@
import org.springframework.batch.core.launch.support.RunIdIncrementer;
import org.springframework.batch.core.repository.JobRepository;
import org.springframework.batch.core.step.builder.StepBuilder;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.transaction.PlatformTransactionManager;
@Configuration
public class AcmeNoopJobConfiguration {
- @Autowired
- private JobRepository jobRepository;
- @Autowired
- private PlatformTransactionManager transactionManager;
- @Autowired
- private AcmeNoopJobTasklet tasklet;
+ private final JobRepository jobRepository;
+ private final PlatformTransactionManager transactionManager;
+ private final AcmeNoopJobTasklet tasklet;
+
+ public AcmeNoopJobConfiguration(JobRepository jobRepository, PlatformTransactionManager transactionManager,
Review Comment:
@galovics
changes have been done.. wating your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "a7med3del1973 (via GitHub)" <gi...@apache.org>.
a7med3del1973 commented on PR #4322:
URL: https://github.com/apache/fineract/pull/4322#issuecomment-2678240646
Thanks for the feedback @adamsaghy !
I understand the need to increase the scope of PRs to maintain a cleaner commit history.
For this PR, I initially focused on refactoring `AcmeNoopJobConfiguration` to ensure the changes were isolated and easy to review. However, I agree that a broader refactor across multiple modules would be more efficient.
Would you prefer I extend this PR to cover additional modules, or should we merge this first and then follow up with a larger refactor in a separate PR? Let me know how you'd like to proceed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "a7med3del1973 (via GitHub)" <gi...@apache.org>.
a7med3del1973 commented on PR #4322:
URL: https://github.com/apache/fineract/pull/4322#issuecomment-2670653833
@galovics
if you think it should be merged
please let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] FINERACT-2081: Refactor constructor injection [fineract]
Posted by "a7med3del1973 (via GitHub)" <gi...@apache.org>.
a7med3del1973 commented on code in PR #4322:
URL: https://github.com/apache/fineract/pull/4322#discussion_r1958684878
##########
custom/acme/loan/job/src/main/java/com/acme/fineract/loan/job/AcmeNoopJobConfiguration.java:
##########
@@ -24,20 +24,23 @@
import org.springframework.batch.core.launch.support.RunIdIncrementer;
import org.springframework.batch.core.repository.JobRepository;
import org.springframework.batch.core.step.builder.StepBuilder;
-import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.transaction.PlatformTransactionManager;
@Configuration
public class AcmeNoopJobConfiguration {
- @Autowired
- private JobRepository jobRepository;
- @Autowired
- private PlatformTransactionManager transactionManager;
- @Autowired
- private AcmeNoopJobTasklet tasklet;
+ private final JobRepository jobRepository;
+ private final PlatformTransactionManager transactionManager;
+ private final AcmeNoopJobTasklet tasklet;
+
+ public AcmeNoopJobConfiguration(JobRepository jobRepository, PlatformTransactionManager transactionManager,
Review Comment:
@galovics
Changes have been made .. waiting your feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org