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

FINERACT-1216: Add FineractClientTest for Fineract SDK REST Client (fix) #1432

Closed
wants to merge 1 commit into from

Conversation

vidakovic
Copy link
Contributor

@ptuomola @vorburger Could one of you quickly have a look at this and merge please? There are 2 small issues with Checkstyle that this PR fixes.
Builds are currently broken because of this one here.

Copy link
Member

@vorburger vorburger left a comment

Choose a reason for hiding this comment

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

Wait a second, then why was #1428 green?! That's super weird, no? (Or was it not?)

https://travis-ci.org/github/apache/fineract/branches says we're currently green on develop

Do you have a link to where the build broken how, so that we can understand what is going on here?

@@ -27,7 +27,7 @@
*
* @author Michael Vorburger.ch
*/
public class Calls {
public final class Calls {
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense, because it's a utility class with a private constructor... (and I vaguely remember that we have a Checkstyle or ErrorProne or SpotBugs check to enforce this - just want to find out why builds don't show up as red - or where they do)

@@ -26,7 +26,7 @@
*
* @author Michael Vorburger.ch
*/
public class FineractClient {
public final class FineractClient {
Copy link
Member

Choose a reason for hiding this comment

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

.. but this shouldn't be required? Especially since in #1433 I'm making createService() a protected method, so this class should remain open (extends-ible).

@@ -42,7 +42,7 @@ public static FineractClientBuilder builder() {
return apiClient.createService(serviceClass);
}

public static class FineractClientBuilder {
public static final class FineractClientBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

as above (I think); best to see failing build and check if all 3 or only 1st (I think) need to be fixed?

@vorburger
Copy link
Member

Did you locally see a failure (running what?) for a task which we are missing on Travis?

I'm curious if e.g. #1431 or #1433 will fail to build on Travis...

@vorburger
Copy link
Member

I'm curious if e.g. #1431 or #1433 will fail to build on Travis...

They passed, so what does this fix? 😈 But I have a suspicion that we may have a larger problem.. we don't have the same quality controls in fineract-client that we have in fineract-provider - do we?

@vidakovic
Copy link
Contributor Author

Already taken care of with #1447

@vidakovic vidakovic closed this Oct 20, 2020
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