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.
Removed this unused import
| @Test | ||
| public void testAdd() { | ||
| QueryParams qp = new QueryParams(); | ||
| qp.add("anothertest", 2); |
There was a problem hiding this comment.
Do we want add to be public to the programmer?
There was a problem hiding this comment.
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.
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.
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.
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.
Yeah, that makes sense. Then should setParams still be public?
There was a problem hiding this comment.
add and setParams should only be available internally
There was a problem hiding this comment.
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
aaron-junot
force-pushed
the
query-params-test
branch
2 times, most recently
from
90ccb73
to
109641b
Compare
aaron-junot
force-pushed
the
query-params-test
branch
from
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.
Exclude from coverage due to the fact that it's a generated file.
Adds a couple of tests around the query params.
setLimitis 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).