#225 Refator PDC.get_modules
Closed by qwan. Opened by qwan.
qwan/freshmaker refactor-pdc-get-modules  into  master

Download 225.patch

Refactor the code to be more readable.

rebased onto 86b582751f818c3cddabb65ccc22c7330c94220e

rebased to add more doc for "latest_only".

I really don't think this is a good idea.

This change limits the call to /modules/ endpoint. Original get_modules is a simple API to call /modules/ endpoint and it is free to pass any filter and ordering arguments, and it can even pass page_size to control how many modules returned from PDC, which can be used to get the top (the latest) module.

From my point of view, get_modules should be the low level API and get_latest_modules should work on top of it, instead of the reverse.

It is not necesary to introduce recursive calls of get_modules for getting latest modules.

Does it work by this call self.session['modules'](page_size=-1, name=name, stream=stream, active=active)?

The latest rebase makes the original inline comment disappear. So, to make it more readable, add that comment again here.

I really don't think this is a good idea.

This change limits the call to /modules/ endpoint. Original get_modules is a simple API to call /modules/ endpoint and it is free to pass any filter and ordering arguments, and it can even pass page_size to control how many modules returned from PDC, which can be used to get the top (the latest) module.

From my point of view, get_modules should be the low level API and get_latest_modules should work on top of it, instead of the reverse.

It is not necesary to introduce recursive calls of get_modules for getting latest modules.

Does it work by this call self.session['modules'](page_size=-1, name=name, stream=stream, active=active)?

To be honest, the original get_modules and get_latest_modules are easy to read and understand. And I can easily pass PDC endpoint arguments to, e.g. page_size, fields and ordering, to optimize. But, it looks things is becoming complicated.

In my opinion, instead of changing code structure, a possible improvement of original get_latest_module would be to document why need to call get_modules in for-loop, and why filter out modules eventually that are not in the modules returned from the first call of get_modules.

This change limits the call to /modules/ endpoint. Original get_modules is a simple API to call /modules/ endpoint and it is free to pass any filter and ordering arguments, and it can even pass page_size to control how many modules returned from PDC, which can be used to get the top (the latest) module.

It's not true, this change makes no difference on this, you can still pass any PDC argument like before.

latest_only is something we're expecting it to be available in PDC, but unfortunately, PDC doesn't have it now, and we need to implement it by ourselves. It's just another query parameter to get_modules like others, so I think it's reasonable to add it here.

The old code, makes it difficult to explain what the latest means, by adding get_latest_module, it's much easier to understand what the latest is, it's not the latest ones in result, but the latest ones in PDC (we need to check modules in PDC to get that), that's easy to understand by the get_latest_module(name, stream) call.

@qwan, @cqi - which way should we go on this one?

I vote -1. Sorry. I don't think those two methods need to be refactored in this way.

np, since this doesn't have impact on functionalities, I'll abandon it at this moment, if we see any necessary of refactoring the code, we can open a new PR.

Pull-Request has been closed by qwan

Metadata