Replace query params in url with params in get

[asheidan]

This replaces the previous implementation of manually calling encoding query params with urlencode with requests.get(url, params=params, ...).

This moves the responsibility of encoding the query parameters to the requests library.

Less responsibility is usually better! ;)

[pyasi]

I agree! Great call, just want to verify that you ensured it works - we currently have a lack of integration testing/CI :(

[pyasi]

@asheidan I've added POST to this class. I like this change would you update your PR to account for POST as well?

[asheidan]

Regarding testing:
I haven’t done exhaustive testing since I’m not familiar enough with the code base or the api it’s communicating with. But I am familiar with requests since I use it as part of my “jobby” job.
I can include tests that cover the changed method, mocking the call to get() and validating the parameters. Hopefully you can expand on those test if you like specific scenarios tested.

Regarding post:
If there’s another method I can of course update the PR covering that as well.

I’ll try to make time for this this evening (UTC+2).

[asheidan]

I've looked at the newly added methods in pybuildkite.client.Client.

Since the new functions adds quite a lot of code duplication I've extracted the actual call to requests to a separate method where you can handle all common scenarios instead.

As I'm not sure if there's a good reason for having separate methods for the different HTTP-verbs in this particular domain, feel free to just close my PR and just use the code from Client.request() in my branch in the other functions.

[pyasi]

I will not be able to merge this unless we change all existing functionality to subscribe to this new pattern. I’m not sure I’m comfortable with having to put a string for each request.

I do however like the consolidation: what about a method in the client for each verb that then passes info to a common request function. This way the classes still call the client.post() directly but the client itself makes any necessary changes and routes it to a request method

[asheidan]

Isn't your suggestion the same as my solution?

If you check my implementation of Client.get():
https://github.com/asheidan/pybuildkite/blob/request_params/pybuildkite/client.py#L61

I was unsure if my solution (with just shims calling Client.request() for each verb would work) or if there were specific needs not yet implemented which needed the totally separated implementations.

[pyasi]

Oh sorry, yes- I was on mobile. This looks great! Thanks for the contribution