-
Notifications
You must be signed in to change notification settings - Fork 23
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
extracted header-magic as enum (bunq/sdk_java#93) #102
Conversation
BunqHeader, and added some junit tests on it
dece499
to
ffb56ad
Compare
private final BunqHeader name; | ||
private final String value; | ||
|
||
public static BunqBasicHeader get(BunqHeader header,Response response) { |
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.
missing space after ,
.
private final BunqHeader name; | ||
private final String value; | ||
|
||
public static BunqBasicHeader get(BunqHeader header,Response response) { |
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.
This is not really a getter but rather a create method, so please rename this method to create
private final String value; | ||
|
||
public static BunqBasicHeader get(BunqHeader header,Response response) { | ||
return new BunqBasicHeader(header,response.header(header.getHeader())); |
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.
missing space after ,
|
||
/** | ||
*/ | ||
public BunqBasicHeader(String name, String value) { |
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.
is there a reason why the constructor is still pubic while you have introduced a create
method. Either make the constructor private/protected or remove the create
method.
this(BunqHeader.parse(name).get(),value); | ||
} | ||
|
||
public BunqBasicHeader(BunqHeader name, String value) { |
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.
same as above.
assertEquals("0 0 0 0 000",headers.get("X-Bunq-Geolocation")); | ||
assertEquals("test-agent",headers.get("User-Agent")); | ||
} | ||
} |
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.
missing new line at EOF
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.
i changed this in BunqHeaderTest, see ad67ada
@@ -54,10 +55,8 @@ public void createAvatarTest() { | |||
|
|||
private String uploadAvatar(byte[] file_contents) { | |||
HashMap<String, String> customHeaders = new HashMap<>(); |
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.
I know you didn't wrote this line, but please refactor to allCustomHeader
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.
np :)
customHeaders.put(ApiClient.HEADER_ATTACHMENT_DESCRIPTION, ATTACHMENT_PATH_IN); | ||
customHeaders.put(ApiClient.HEADER_CONTENT_TYPE, CONTENT_TYPE); | ||
|
||
BunqHeader.attachmentDescription.addTo(customHeaders,ATTACHMENT_PATH_IN); |
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.
missing space
customHeaders.put(ApiClient.HEADER_CONTENT_TYPE, CONTENT_TYPE); | ||
|
||
BunqHeader.attachmentDescription.addTo(customHeaders,ATTACHMENT_PATH_IN); | ||
BunqHeader.contentType.addTo(customHeaders,CONTENT_TYPE); |
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.
missing space
return AttachmentPublic.create(customHeaders, file_contents).getValue(); | ||
} | ||
|
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 revert the removal of this line.
i've processed the formatting thingies :) |
i can't really see what's up with com.bunq.sdk.model.generated.endpoint.CardDebitTest > orderNewMaestroCardTest FAILED but i think the formatting-relevant-header stuff is functional now; i intent to add some more tests with relevant request/response header content to validate that |
@tubbynl there are still some comments open. It seems that your commit messages are still not passing. Your missing |
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.
There are still a few unresolved old comments
@@ -320,10 +285,7 @@ private BunqResponseRaw createBunqResponseRaw(Response response) | |||
*/ | |||
private static String getResponseId(Response response) { | |||
Map<String, String> headerMap = getHeadersMap(response); | |||
|
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.
This new line should come back
public class BunqBasicHeader { | ||
private String name; | ||
private String value; | ||
private static final String DELIMITER_HEADER_NAME_AND_VALUE = ": "; |
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.
Missing constant group doc block.
In this case it can be
/**
* String format constants.
*/
and perpend these 2 constants with FORMAT_
*/ | ||
public BunqBasicHeader(String name, String value) { | ||
public static Optional<BunqBasicHeader> get(String header, String value) { | ||
return BunqHeader.parse(header).map(h -> new BunqBasicHeader(h,value)); |
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.
This is not java 7 compatible. Please refactor this so that it is.
And please add missing space after ,
0bca993
to
0ebd723
Compare
i extracted magic constants into BunqHeader enum, added some junit tests on it
behaviour is as-is, but tests are not really runnable locally
i'm using the bunq api currently in a spring-boot setup using rest-template, most of the gson/models i can use for deserializing the responses and in order to mimic the header handling i needed to copy some of the header names into my project.
i extracted that into an BunqHeader enum which you see here, if this get's into the sdk_java project i can remove this from my project :)