-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add test for query params #27
Conversation
@@ -6,7 +6,6 @@ | |||
import com.recurly.v3.fixtures.MyRequest; | |||
import com.recurly.v3.fixtures.MyResource; | |||
import org.junit.jupiter.api.Test; | |||
import com.recurly.v3.ApiException; |
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.
Removed this unused import
@Test | ||
public void testAdd() { | ||
QueryParams qp = new QueryParams(); | ||
qp.add("anothertest", 2); |
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.
Do we want add
to be public to the programmer?
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 thought we did in case we wanted to allow for other arbitrary query params that aren't specified by the API? If not, I can make it private and remove the test
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.
Oh, you know what? The programmer should be able to add arbitrary query params by specifying them in the HashMap and then setting them with setParams
, so even if we made add
private, it wouldn't block them from doing that. I still think it's a cool little utility method, but they could work around it by using getParams
, adding to the HashMap and then setting it again with setParams
.
I'll go ahead and make it private for now, but if someone wants us to make it public someday, I think we ought to go ahead and give them that.
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'm worried people might use this over the named / typed methods. Also, if something is missing, that needs to be addressed in the generator.
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.
Suppose someone does params.add("limit", 200)
for instance, and we change the name of "limit" to "pageSize" in the next version. The compiler can't help them.
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.
Yeah, that makes sense. Then should setParams
still be public?
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.
Ideally, no.
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.
add
and setParams
should only be available internally
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.
Ok. The params hashmap is created when the object is instantiated. getParams
will need to be public still, but I don't think anything actually uses setParams
at this time. I might be able to just remove it altogether if we're not trying to provide that interface to the programmer
90ccb73
to
109641b
Compare
109641b
to
04f1e75
Compare
@@ -54,6 +54,7 @@ | |||
<exclude>**/v3/resources/*</exclude> | |||
<exclude>**/v3/Client.*</exclude> | |||
<exclude>**/v3/exception/*</exclude> | |||
<exclude>**/v3/QueryParams.*</exclude> |
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.
Exclude from coverage due to the fact that it's a generated file.
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.
👍
Adds a couple of tests around the query params.
setLimit
is the only portion that's truly generated (everything else that I'm testing here is written directly into the generator template). The rest of the dynamic methods do not have tests around them, but there's no way to exclude them from coverage unless we exclude the entire class (which may be the best option, but I haven't done that just yet).