Details

I noticed that only PIDS can be provided to the content, directory, revision, release and snapshot methods.
I imagine this is intended for validation purposes. But it could be great to also allow to pass hashes to these methods
as we can easily reconstruct the PIDS from them when we know the object type.

Also a nitpick: for consistency with the other swh modules docstrings, sentences should begin with an upper case letter.

Next step: add a CLI tool wrapping that web api client ?

requirements-swh.txt
2–3	You can remove the `swh.core` dependency.
swh/web/client/client.py
176–189 ↗	(On Diff #10147)	You could move that block in the class constructor and create a `_getters` instance variable.
swh/web/client/tests/conftest.py
23 ↗	(On Diff #10147)	remaining debug output I guess
swh/web/client/tests/test_web_api_client.py
13 ↗	(On Diff #10147)	You should also test `web_api_client.content(pid)` result.
25 ↗	(On Diff #10147)	You should also test `web_api_client.directory(pid)` result.
39 ↗	(On Diff #10147)	You should also test `web_api_client.release(pid)` result.
54 ↗	(On Diff #10147)	You should also test `web_api_client.revision(pid)` result.

This revision now requires changes to proceed.Mar 19 2020, 3:22 PM

requirements.txt: drop redundant dep on swh.core
client.py: move getters table for .get() to an instance variable
pytest fixture: remove left-over debug print
tests: also test type-specific getters

Build is green
See https://jenkins.softwareheritage.org/job/DWCLI/job/tox/4/ for more details.

Harbormaster completed remote builds in B11220: Diff 10154.Mar 19 2020, 4:04 PM

client.py: make docstring start with uppercase letters

Build is green
See https://jenkins.softwareheritage.org/job/DWCLI/job/tox/5/ for more details.

Harbormaster completed remote builds in B11221: Diff 10155.Mar 19 2020, 4:07 PM

Thanks for the great review !

All done, except for this point, which was indeed on purpose:

I noticed that only PIDS can be provided to the content, directory, revision, release and snapshot methods.

not really for validation purposes (although it clearly helps with that), but rather because a background idea here is to be future-proof, and already adopt a convention that we have on the roadmap for API v2, i.e., use PIDs (and only PIDs) everywhere.

swh/web/client/client.py
176–189 ↗	(On Diff #10147)	good idea, it also allows to factor out the _get_snapshot() method, that might be useful elsewhere
swh/web/client/tests/conftest.py
23 ↗	(On Diff #10147)	indeed, oops :-)
swh/web/client/tests/test_web_api_client.py
13 ↗	(On Diff #10147)	right, I didn't want to duplicate test logic, but I think I've now found a way to test both ways of retrieving objects without doing so (same answer applies to all other comments like this one)

not really for validation purposes (although it clearly helps with that), but rather because a background idea here is to be future-proof, and already adopt a convention that we have on the roadmap for API v2, i.e., use PIDs (and only PIDs) everywhere.

Ack, I was missing some context.

Otherwise, looks good to me !