Page MenuHomeSoftware Heritage

Autogenerate the Indexer Storage HTTP API.
ClosedPublic

Authored by vlorentz on Oct 5 2018, 6:25 PM.

Details

Summary

Pros:

  • Less duplication
  • Sphinx shows methods docstring on the client API as well

Cons:

  • Some Python magic
  • Won't play well with static analysis tools.

Depends on D507

Diff Detail

Repository
rDCIDX Metadata indexer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

This comment was removed by vlorentz.

I'm not sure I'm a huge fan of some of the hacks required (removing the cur/self/... arguments), but on the purely technical side this looks very good.

The only thing I would question is whether it would be worth it to start directly by factorizing that in the RPC itself, in order to force ourselves finding generic solutions that would work for the rest of the remote APIs?

I'm not sure I'm a huge fan of some of the hacks required (removing the cur/self/... arguments),

How else can we avoid this though?

Cons:

Some Python magic
Won't play well with static analysis tools.

People might as well.
I do not see another way to factorize some more though.

Also, tests are fine! So that's a good sign it does what i think it does.

I don't think it's clear to me what happens during import and the implications here (since we run code here to declare methods/functions/class, instead of simply declaring those).
That might be my biggest concern.

In that regard, that might be a clue to push further below that kind of code...

The only thing I would question is whether it would be worth it to start directly by factorizing that in the RPC itself, in order to force ourselves finding generic solutions that would work for the rest of the remote APIs?

... (as seirl hinted at)...

Cheers,

swh/indexer/storage/__init__.py
23

Don't we want to keep the original docstring as well here (@functools.wraps)?

swh/indexer/storage/api/client.py
21

I do not see that snippet being called. If it's not, can you please remove it?

vlorentz added inline comments.
swh/indexer/storage/__init__.py
23

The original function is returned, so there is no wrapper.

swh/indexer/storage/api/client.py
21

Good catch; that was for an intermediate version of the Diff.

vlorentz marked an inline comment as done.
  • Remove dead code.
swh/indexer/storage/__init__.py
23

Right!

In D505#9661, @seirl wrote:

The only thing I would question is whether it would be worth it to start directly by factorizing that in the RPC itself, in order to force ourselves finding generic solutions that would work for the rest of the remote APIs?

Indeed. I just moved all the code to swh.core.api and it looks even nicer this way (no need to put the for loop at the root of a module). I'll write a Diff later today.

  • Move the interesting parts of this Diff into the core (see D507).

In this current state, i have errors on make test, the base of which seems to be this:

TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

steps to reproduce:

cd swh-core
arc patch D507
cd ../swh-indexer
arc patch D505
make test

In this current state, i have errors on make test, the base of which seems to be this:

TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

Details are at P312

TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

Details are at P312

Well, it was a side-step on my part, i needed to arc patch the D513 on the objstorage as well.
Tests are fine after that.

This revision is now accepted and ready to land.Oct 11 2018, 11:43 AM