Details

swh/core/api.py
23–29	not sure I get the reason for this to be a class. IMHO it should be a plain decorator, the class here seems to be just a namespace... and I tend to avoid unnecessary inheritance, and this looks to me as a source of unnecessary class heritage.

vlorentz marked an inline comment as done.Oct 10 2018, 9:33 AM

vlorentz added inline comments.

swh/core/api.py
23–29	The idea is that in the future we may need to extend it; but I don't have a strong opinion one way or the other.

ardumont added inline comments.Oct 10 2018, 11:01 AM

swh/core/api.py
23–29	Here is a clue, if we do not need it right now, keep the original one (function as [1]). We will change when the need comes ;) And indeed, the function name `endpoint` becomes less clear, `remote_endpoint` seems better. Also, note that this pattern of remote api is not necessarily for storage api (cf. vault, scheduler). [1] https://forge.softwareheritage.org/D505?vs=on&id=1498&whitespace=ignore-most#inline-2433
169	Mention in the docstrings the implementations example so that when the documentation is generated, we can follow those samples. See for example :class:`IndexerStorage`, :class:`TestStorage`, etc...
171	Mention IndexerStorage as an example... I'd change the sentence to: '...with @endpoint in potential remote api (e.g :class:`IndexerStorage`, etc...)'
221	Mention what the args are. I like how you type your arguments by the way (not sure it's quite clear we can add type here though ;)

Apply @ardumont's suggestions.

Harbormaster completed remote builds in B1605: Diff 1574.Oct 10 2018, 11:13 AM

fwiw, i'm waiting on the metaclass implementation since the discussion conclusion converged on this:

irc discussion:

09:29:30 douardda_:   vlorentz: I wonder if a metaclass would not have been simpler there D507
09:29:31 --           Notice(swhbot): D507 (author: vlorentz, Needs Review) on swh-core: Add functions to generate HTTP API clients and servers from databases. <https://forge.softwareheritage.org/D507>
09:31:49 vlorentz:    douardda_: a metaclass can't take an argument
09:32:10 vlorentz:    and we need to pass the Storage class somehow
09:45:45 douardda_:   a decorated metaclass then! (just kidding)
09:46:16 vlorentz:    You mean a metaclass factory
09:47:10 vlorentz:    A metametaclass would be the "right" way to do it, but ugh
09:49:36 douardda_:   ;)
11:24:27 +seirl:      vlorentz: huh? you can totally do MyClass(metaclass=MyMeta, arg1=42), unless I'm misunderstanding what you're saying
11:24:36 vlorentz:    seirl: ooooh
11:24:46 vlorentz:    seirl: indeed, I totally forgot that
11:25:07 +seirl:      it's easier in 3.6 but it works well in 3.x
11:25:10 vlorentz:    I guess I'll make it a metaclass then

Cheers,

ardumont added a child revision: D505: Autogenerate the Indexer Storage HTTP API..Oct 11 2018, 9:37 AM

Move the endpoint generation from decorators/functions to existing base classes.

Harbormaster completed remote builds in B1616: Diff 1590.Oct 11 2018, 9:38 AM

Better name.

Harbormaster completed remote builds in B1617: Diff 1591.Oct 11 2018, 9:40 AM

Fix class name.

Harbormaster completed remote builds in B1618: Diff 1592.Oct 11 2018, 9:59 AM

vlorentz mentioned this in D513: Fix RemoteObjStorage to work with SWHRemoteAPI's metaclass..Oct 11 2018, 10:13 AM

vlorentz added a child revision: D513: Fix RemoteObjStorage to work with SWHRemoteAPI's metaclass..Oct 11 2018, 10:14 AM

ardumont added inline comments.Oct 11 2018, 10:40 AM

swh/core/tests/test_api.py
9	You need to rebase your diff to the latest swh-core. When patching, i did not see the requirements-test.txt file. Which then needs also patching to add the requests-mock dependency ;)