Skip to content

Conversation

@dan-s1
Copy link
Contributor

@dan-s1 dan-s1 commented Dec 2, 2024

Summary

NIFI-14051
This PR aims to replace explicit type arguments with diamonds. In addition various improvements to the related unit tests were made such as specific assertions, removal of exceptions from method signatures which are not thrown, the use of later Java features such as getFirst on a list and the use of Map.of instead of instantiated HashMap with anonymous population.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@exceptionfactory exceptionfactory 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 working on these improvements @dan-s1. The changes are helpful in general, but the scope makes it rather difficult to review since it includes different types of changes that cover a number of vital classes.

The initial removal of unnecessary generic types is straightforward, but replacing anonymous classes with lambdas impacts a large number of lines. For this reason, it would be helpful to break these changes into separate pull requests. One way to do this would be to back out changes in the nifi-framework-bundle from the current PR and handle them separately. Another way to do it would be to limit the types of changes. However, I think separating framework changes into a separate PR would help ensure sufficient review of more important components.

These types of stylistic improvements are useful, and we should look at formalizing some of these changes using static analysis tools like PMD. However, for future reference, having a more narrow scope would help the review process and reduce the potential for missing important changes.

@dan-s1
Copy link
Contributor Author

dan-s1 commented Dec 6, 2024

Thanks for working on these improvements @dan-s1. The changes are helpful in general, but the scope makes it rather difficult to review since it includes different types of changes that cover a number of vital classes.

The initial removal of unnecessary generic types is straightforward, but replacing anonymous classes with lambdas impacts a large number of lines. For this reason, it would be helpful to break these changes into separate pull requests. One way to do this would be to back out changes in the nifi-framework-bundle from the current PR and handle them separately. Another way to do it would be to limit the types of changes. However, I think separating framework changes into a separate PR would help ensure sufficient review of more important components.

These types of stylistic improvements are useful, and we should look at formalizing some of these changes using static analysis tools like PMD. However, for future reference, having a more narrow scope would help the review process and reduce the potential for missing important changes.

I will separate the changes into two tickets. Initially I thought they were somewhat related but I am perfectly fine making two tickets for these changes.

@dan-s1
Copy link
Contributor Author

dan-s1 commented Dec 6, 2024

@exceptionfactory You were right it is much easier to see the changes when the changes are focused only where the explicit type arguments are replaced by diamonds. I also noticed though I made a lot of non related changes to unit tests per Intellij suggestions in those unit tests where I had to replace explicit type arguments with diamonds. I know on a previous PR you were okay with me cleaning up the unit tests so I took the liberty to do so when touching these unit tests. Please let me know if that is okay. Thanks!

@dan-s1 dan-s1 changed the title NIFI-14051 Replaced type arguments with diamonds, replaced anonymous classes with lambdas and replaced certain assertions with more specific assertions. NIFI-14051 Replaced type arguments with diamonds Dec 6, 2024
@dan-s1
Copy link
Contributor Author

dan-s1 commented Dec 6, 2024

Odd that TestGenerateTableFetch.testInitialMaxValueWithELAndMultipleTables failed only on the Windows box as there was attempt to modify a Map created with Map.of which can not be added to. I would have thought it would fail on all boxes.

Copy link
Contributor

@exceptionfactory exceptionfactory 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 narrowing the scope of changes @dan-s1, the latest round was much easier to go through. Great to have this code cleanup in a number of places! +1 merging

@exceptionfactory exceptionfactory merged commit ba9273f into apache:main Dec 6, 2024
9 checks passed
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