Page MenuHomeSoftware Heritage

add Python client for the Web API
ClosedPublic

Authored by zack on Wed, Mar 18, 7:02 PM.

Diff Detail

Repository
rDWCLI Web client
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

zack created this revision.Wed, Mar 18, 7:02 PM
zack updated this revision to Diff 10144.Wed, Mar 18, 7:06 PM
  • Web API client: first implementation
  • Web API client: add tests
zack updated this revision to Diff 10147.Wed, Mar 18, 8:11 PM
  • web client tests: add missing copyright headers
anlambert requested changes to this revision.Thu, Mar 19, 3:22 PM

I have added a couple of inline comments that should be addressed before landing the client.

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.Thu, Mar 19, 3:22 PM
zack updated this revision to Diff 10154.Thu, Mar 19, 4:04 PM
zack marked 7 inline comments as done.
  • 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
zack updated this revision to Diff 10155.Thu, Mar 19, 4:06 PM
  • client.py: make docstring start with uppercase letters
zack added a comment.Thu, Mar 19, 4:08 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)

anlambert accepted this revision.Thu, Mar 19, 4:27 PM

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 !

swh/web/client/tests/test_web_api_client.py
38 ↗(On Diff #10155)

nice !

This revision is now accepted and ready to land.Thu, Mar 19, 4:27 PM