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

more SpotBugs related / inspired code clean up (see FINERACT-702) #684

Merged
merged 1 commit into from
Jan 14, 2020
Merged

more SpotBugs related / inspired code clean up (see FINERACT-702) #684

merged 1 commit into from
Jan 14, 2020

Conversation

vorburger
Copy link
Member

@vorburger vorburger commented Jan 11, 2020 •
edited
Loading

based upon code review of changes made by @awasum in previous commit:

  • fix clear contains() GC_UNRELATED_TYPES bug in Loan

  • fix clear equals() EC_UNRELATED_TYPES bug in Loan

  • fix more equals() & hashCode() in...

    • Charge
    • TrialBalance
    • LoanProductBorrowerCycleVariations
    • LoanProductProvisioningEntry

  • fix AccountingConstants to avoid using @FindBugsSuppressWarnings

  • fix SelfLoansDataValidator instead of @FindBugsSuppressWarnings

  • fix up weird static in GLAccountBuilder

  • minor clean up in build.gradle (spotbugs has replaced findbugs)

PS: This change originally included also fixing the missing hashCode() and equals() in ShareAccountCharge, but that turned out to be non trivial, see FINERACT-827, and is thus not yet fixed by this.

@awasum
Copy link
Contributor

awasum commented Jan 11, 2020

Is this still relevant?

@vorburger vorburger mentioned this pull request Jan 12, 2020
Merged
@vorburger vorburger requested a review from awasum January 12, 2020 12:33
@vorburger
Copy link
Member Author

Is this still relevant?

Yes, very much so; this contains more but new SpotBugs related / inspired clean up which I coded on top of your recent work! See #685 re. how this was meant to be reviewed as an addendum to #682 .. now that is in, the diff will look clearer. The Conflicts shown are bogus (I don't really understand why GitHub is that stupid; a simple git rebase on the CLI fixes them).

I'm just looking into why ShareAccountIntegrationTests (only) fails on this PR. Maybe I'll break it up into smaller ones.

@vorburger
Copy link
Member Author

why ShareAccountIntegrationTests (only) fails on this PR

specifically testCreateShareAccountWithCharges() in line 487 Assert.assertEquals("0.0", String.valueOf(chargeDef.get("amountOutstanding"))); ... I've manually reverted some of the changes of this PR to understand which one causes that failure, and figured it must be related to one of those hashCode() and equals() changes in Charge, TrialBalance, LoanProductProvisioningEntry, LoanProductBorrowerCycleVariations or ShareAccountCharge. Hm.

@vorburger
Copy link
Member Author

The ShareAccountIntegrationTests.testCreateShareAccountWithCharges() failure was related to ShareAccountCharge, something is really wrong there... to move forward with getting all the rest of the changes proposed here merged ASAP, I've reverted the hashCode() and equals() changes in ShareAccountCharge from here for now; let's pick that up in dedicated FINERACT-827. With that, I expect that this PR will now pass CI IT (unless it's Sunday, in which case it will fail due to FINERACT-820, which is totally unrelated to everything else here).

@vorburger
more SpotBugs related / inspired code clean up (see FINERACT-702)
6ca1a67 
based upon code review of changes made by @awasum in previous commit:

 - fix clear contains() GC_UNRELATED_TYPES bug in Loan

 - fix clear equals() EC_UNRELATED_TYPES bug in Loan

 - fix more equals() & hashCode() in...
    * Charge
    * TrialBalance
    * LoanProductBorrowerCycleVariations
    * LoanProductProvisioningEntry

 - fix AccountingConstants to avoid using @FindBugsSuppressWarnings

 - fix SelfLoansDataValidator instead of @FindBugsSuppressWarnings

 - minor clean up in build.gradle (spotbugs has replaced findbugs)

PS: This change originally included also fixing the missing hashCode()
    and equals() in ShareAccountCharge, but that turned out to be non
    trivial, see FINERACT-827, and is thus not yet fixed by this.
@vorburger
Copy link
Member Author

@awasum this now passed the build (because it's Monday), and is finished and ready from my side.

Will you review and merge this one when you have a moment?

@vorburger vorburger closed this Jan 13, 2020
@vorburger vorburger reopened this Jan 13, 2020
@vorburger vorburger mentioned this pull request Jan 14, 2020
Merged
awasum
awasum approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@awasum awasum left a comment

Choose a reason for hiding this comment

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

LGTM, Merging now.

@awasum awasum merged commit fc786fa into apache:develop Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awasum awasum awasum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@vorburger @awasum