docs(rest_client): note about data_selector

[joscha]

I personally think it's a bug and

if self.data_selector:
    return self.extract_response(response, self.data_selector)

should be called automatically for all requests (.get(...).json(), ...) when given (1), however it would be a breaking change, so updating the documentation is the next best thing.
Might also be possibly worth deprecating the parameter from the global RESTClient constructor, given the fact that it is only used inside .paginate(...) (2).

Let me know if you want me to open a pull request for (1) and/or (2) instead of this one.

[netlify]

✅ Deploy Preview for dlt-hub-docs canceled.

Name	Link
🔨 Latest commit	343dc8d
🔍 Latest deploy log	https://app.netlify.com/sites/dlt-hub-docs/deploys/674867165286830008667df7

[burnash]

Hey @joscha you're absolutely right. I'd say it should not be in RESTClient.__init__ indeed. I would opt for (2) as I don't think it's really needed for get() since the data (if it's a JSON response) can be retrieved directly from the resulted object.
Thanks for spotting this!

[joscha]

Do you want me to remove it from Rest client? It is a breaking change, albeit a super easy fix. Alternatively I could deprecate it instead and leave the comment in this PR.

[burnash]

Do you want me to remove it from Rest client? It is a breaking change, albeit a super easy fix. Alternatively I could deprecate it instead and leave the comment in this PR.

Let's deprecate it now (with the DeprecationWarning) and we'll remove it with the next major release.

[joscha]

Do you want me to remove it from Rest client? It is a breaking change, albeit a super easy fix. Alternatively I could deprecate it instead and leave the comment in this PR.

Let's deprecate it now (with the DeprecationWarning) and we'll remove it with the next major release.

Okay. Will update the PR.

[joscha]

Whilst doing the update I just realized that this was probably introduced for convenience. Like for example an API with lots of paginated endpoints that all follow the same pattern. It allows to do something like:

rest_client = RESTClient(..., data_selector="data")

yield from rest_client.paginate("a")
yield from rest_client.paginate("b")
yield from rest_client.paginate("c")
...
yield from rest_client.paginate("x")

without having to pass the data_selector="data" to each of them:

rest_client = RESTClient(...)

yield from rest_client.paginate("a", data_selector="data")
yield from rest_client.paginate("b", data_selector="data")
yield from rest_client.paginate("c", data_selector="data")
...
yield from rest_client.paginate("x", data_selector="data")

Which is definitely more brittle and less DRY.
Whilst it being a bit confusing to not be respected in get/... I actually do think after looking at the code, that there's validity in this convenience, so I'd propose to not deprecate it but leave the comment as-is. WDYT @burnash?

[burnash]

I actually do think after looking at the code, that there's validity in this convenience, so I'd propose to not deprecate it but leave the comment as-is.

Yes, also makes sense. Let's do that. I have a suggestion to adjust the wording though.

[burnash]

Thank you, @joscha!