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

Migrate _handler to __init__.py #256

Closed
Zeitsperre opened this issue Jul 24, 2018 · 12 comments
Closed

Migrate _handler to __init__.py #256

Zeitsperre opened this issue Jul 24, 2018 · 12 comments

Comments

@Zeitsperre
Copy link
Contributor

Zeitsperre commented Jul 24, 2018

def _handler(self, request, response):

For several process 'groups', much of the code is redundant, specifically when it comes to the _handler functions. Would it not make more sense to migrate the _handlers to __init__.py as empty classes and then build on them using the processes? e.g.

from pywps import Process
from flyingpigeon.processes import _subset_handler

class SubseteuropeProcess(Process)
    ...
    super(SubseteuropeProcess, self).__init__(
    self._subset_handler,
    ...
    )

I've never seen super before but it seems to simply be a class-like function so so long as the _handler is defined, there should be no problem moving it.

@huard
Copy link
Contributor

huard commented Jul 24, 2018

Maybe not in init, but in a generic subset.py module.

@Zeitsperre
Copy link
Contributor Author

The subset group of Processes is not the only one to make use of these handlers. We could specify a file with a leading underscore (like _handlers.py) that contains all the internal _handler classes that shouldn't be exported.

It's worth taking a closer look at whether or not we can produce a more generic _handler class instance for all Processes (would help simplify the action of creating new Processes).

@Zeitsperre
Copy link
Contributor Author

Also, I do agree that having a more generic subset.py script would be a good place to store the base subset class. Much of the code between th 4-5 versions is the same.

@huard
Copy link
Contributor

huard commented Jul 24, 2018

Agree with reducing boilerplate code, but not convinced about the solution proposed. We want the logic of the process to be included in the code, so it's readable. I would rather suggest writing general utilities that are called by the _handler.

@Zeitsperre
Copy link
Contributor Author

Zeitsperre commented Jul 24, 2018

That's a good point. Simplifying the code should not obfuscate the algorithm logic.

When you mean wriitng utilities that _handler calls, are you suggesting creating things like LOGGER-initializing superclasses? I'm not sure how else to simplify the _handler class.

@huard
Copy link
Contributor

huard commented Jul 24, 2018

Hum, rather taking code repeated in multiple _handler methods and trying to abstract it in generic functions.

@cehbrecht
Copy link
Member

cehbrecht commented Jul 31, 2018

There was already a discussion about how we can reduce duplication of code. One way we already started is to use the eggshell for shared functionality.

There was a discussion on the pywps-dev mailing list started by David.

There are also other tickets on this subject: #234 and bird-house/cookiecutter-birdhouse#3

@cehbrecht cehbrecht added this to the 1.3 milestone Jul 31, 2018
@cehbrecht cehbrecht added this to To Do in Dar-es-Salaam: September 2018 Release via automation Jul 31, 2018
@cehbrecht
Copy link
Member

I still didn't come up with an example by myself ... but I think Python decorators might also be a way to reduce code duplication. Didn't find a good example, but at least one article:
https://hackernoon.com/decorators-in-python-8fd0dce93c08

@cehbrecht cehbrecht added this to To do in Washington: December 2018 Release via automation Dec 3, 2018
@cehbrecht cehbrecht removed this from the 1.3 milestone Dec 3, 2018
@nilshempelmann
Copy link
Member

@huard reading that thread I would propose to migrate 'subset' module from Flyingpigeon to 'eggshell.nc.subset'. As well moving the corresponding shapefiles from flyingpigen into 'eggshell/data/shapefiles/'
Gues subset functions could be usefull in 'raven' and 'finch' as well.

@huard
Copy link
Contributor

huard commented Dec 12, 2018

I'm -1 on this until Finch or Raven actually implement subsetting and decide to use the FP functionality.

@nilshempelmann
Copy link
Member

Ok. Great.
Than the current design is the right one. No need for change.
Can this issue be closed?

@Zeitsperre
Copy link
Contributor Author

Closing this for now.

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

No branches or pull requests

4 participants