-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feature/callback models bunq/sdk java#33 #36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments
|
||
@Test | ||
public void bunqMeTabModelTest() throws InvocationTargetException, FileNotFoundException, | ||
ClassNotFoundException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our unit of indentation in Java is two spaces, and two more for hanging code. It seems the indents are wider here %)
Please take a look at the rest of the SDK code and format consistently with it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My indention settings was on 4 😊 Changed to 2 and reformatted code.
private void executeNotificationUrlTest( | ||
String expectedJsonFileName, | ||
String classNameExpected, | ||
String referencedObjectGetterName) throws FileNotFoundException, NoSuchMethodException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multi-line argument formatting should have closing braces on new lines:
private void executeNotificationUrlTest(
String expectedJsonFileName,
String classNameExpected,
String referencedObjectGetterName
) throws FileNotFoundException, NoSuchMethodException,
Please fix this throughout the changes as well %)
private static final String JSON_PATH_SHARE_INVITE_BANK_RESPONSE_MODEL = BASE_PATH_JSON_MODEL + | ||
"/ShareInviteBankResponse.json"; | ||
|
||
private static final Type NOTIFICATION_URL_TYPE = new TypeToken<NotificationUrl>(){}.getType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us call it TYPE_NOTIFICATION_URL
(from general to specific)
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the JSON files in java seem to have too many newlines in the end. It does not matter that much, but would be cool if you could fix it for consistency anyways :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments :D
import static org.junit.Assert.assertTrue; | ||
|
||
public class NotificationUrlTest extends BunqSdkTestBase { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a newline before the class body beginning
"end_date": "2017-11-11 02:34:05.864029" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Json (all of it) should still be indented with 4 spaces xD
private static final Type TYPE_NOTIFICATION_URL = new TypeToken<NotificationUrl>() {}.getType(); | ||
|
||
private void executeNotificationUrlTest( | ||
String expectedJsonFileName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take care of the hanging indent too. It should be 4 in our case I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Closes #33
Closes #37