Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactore roulier to be able to choose between multiple implementation for one carrier #140

Open
florian-dacosta opened this issue Feb 15, 2021 · 5 comments

Comments

@florian-dacosta
Copy link
Member

For some carrier, we may want to manage different services/implementation.
Gls France for instance propose multiple solutions, like GLS-BOX, which return zpl, or GLS Web API.
Laposte fr also have multiple solutions (SOAP XML, the current implementation, or a REST API, not implemented yet)
DPD is also changing its api, the current one should be abandoned soon, etc...

It seems it could be quite usefull to ba able to choose among mulltipe implementation of a service/action.
For example to enjoy new functionalities or during the transition from one system to another, etc...

We currently have the case for GLS, where present implementation is the GLS-BOX and we want to also implementent the GLS Web API one : #138.

Since one carrier may have a propose different solution depending on the country (typlically, the implemented GLS-BOX is only for France and won't work for other countries), we had previously decided to add a suffix depending the country to be able to manage the carrier in multiple countries.
So, the gls_eu carrier type proposed in #138 is really confusing (unless it work for multiple european countries, but I doubt it..not sure though.

I'd like then to introduce a system allowing to choose between 2 implementation, while having a default one (so the user that do not care about which implementation is used to get a label don't need to care about that).

For this, we could modify the factory sytem to have something like that :

class RoulierFactory(object):
    def __init__(self):
        self._carrier_action = {}

    def register_builder(self, carrier_type, action, Carrierclass, ttype="default"):
        self._carrier_action[(carrier_type, action)] = {ttype: Carrierclass}

    def get(self, carrier_type, action, ttype="default, **kwargs):
        carrierclass = self._carrier_action.get((carrier_type, action), {}).get(ttype)
        if not carrierclass:
            raise ValueError((carrier_type, action))
        return carrierclass(carrier_type, action, **kwargs)

def get(carrier_type, action, *args, **kwargs):
    carrier_obj = factory.get(carrier_type, action)
    ttype = kwargs.pop('ttype', 'default')
    return getattr(carrier_obj, action)(carrier_type, action, *args, **kwargs)

And on carrier side :

class GlsFrBoxGetLabel(CarrierGetLabel):
...

factory.register_builder("gls_fr", "get_label", GlsFrBoxGetLabel, ttype="gls-box")

class GlsWebGetLabel(CarrierGetLabel):
...

factory.register_builder("gls_fr", "get_label", GlsWebGetLabel)

This way, calling roulier.get('gls_fr', 'get_label', payload) will return a gls label using the WEB api
and calling roulier.get('gls_fr', 'get_label', payload, ttype="gls-box") will return a gls label using the GLS-BOX solution.

Any comment about such a change ?
@DylannCordel I'd appreciate your opinion about this

cc @hparfr @bealdav

@DylannCordel
Copy link
Contributor

DylannCordel commented Feb 18, 2021

Hi,

I think it's a good idea but the "default" could be problematic. IMHO, user should explicitly chose the solution he wants to use because data are not always the same (credentials for GLS REST are not the same as for GLS BOX for eg.). But it means it won't be backward-compatible if we force the user to chose the solution.

+1 to have a naming convention as <carrier>_<countrycode>_<api_type> : gls_fr_rest, gls_fr_glsbox etc.

In this way, if we want to force the choice of the solution, there is no changes to do except renaming current carriers folders (and their registration).

With this change, it could be great to split those subdirectories into external repositories to be able to:

  • only install what we need
  • avoid dead code in the master branch

eg: akretion/roulier, akretion/roulier-gls-fr-rest, akretion/roulier-gls-fr-glsbox etc.

setup.py could be configured to have optionnal dependency to easily choose which carriers (officially supported/maintained) by akretion) we want to auto-install.

@bealdav
Copy link
Member

bealdav commented Feb 18, 2021

Nice to see gls_fr_rest, gls_fr_glsbox convention

setup.py could be configured to have optionnal dependency to easily choose which carriers we want to auto-install.

Don't you think it could be confusing to have so many configuration cases ? @DylannCordel

What do you think about a lib by carrier with plugin management ?

@florian-dacosta
Copy link
Member Author

florian-dacosta commented Feb 18, 2021

@DylannCordel
The idea of the 'default', was not to add complexity for the majority of the carrier fr which we have only one implementation.
But yes the developper of the app using roulier should know what implementation he is using so maybe it is clearer and easier the way you propose (which does not require change in base roulier actually)

So I guess for a start, we can use the convention gls_fr_rest, gls_fr_rest.

@DylannCordel
Copy link
Contributor

DylannCordel commented Feb 22, 2021

@bealdav I thought to update the setup.py to inform pip there are some extra:

extras = {
    'gls-fr': ['roulier-gls-fr', 'andSomeOthersDepsIfNeeded'],
    'laposte-fr': ['roulier-laposte-fr', 'andSomeOthersDepsIfNeeded'],
    'dpd-fr': ['roulier-dpd-fr', 'andSomeOthersDepsIfNeeded'],
    'some-uk': ['roulier-some-uk', 'andSomeOthersDepsIfNeeded'],
}
extras['all'] = list(set(dep for opt_deps in extras.values() for dep in opt_deps))
extras['all-fr'] = list(set(dep for opt, opt_deps in extras.items() for dep in opt_deps if opt.endswith('-fr')))

setup(
    # ...
    extras_require=extras,
)

When you install roulier you can choose to install only what you need. For exemple, only GLS and DPD:

pip install roulier[gls-fr,dpd-fr]

And if we want all french "officially" supported carriers:

pip install roulier[all-fr]

Or all "officially" supported carriers (fr, uk etc.):

pip install roulier[all]

What do you think about a lib by carrier with plugin management ?

I'm not sure to understand your proposal. Do you mean to have a shared / common base for a carrier (the API and eventually some tests) and have the technical solution (transport, encoding, decoding) as "plugin" ?

@bealdav
Copy link
Member

bealdav commented Feb 22, 2021

Use extras_require can be a good step, thanks. @florian-dacosta ?
It's sufficient to cover that I thought by plugin, no need to more decouple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants