bird-house / flyingpigeon

WPS processes for climate model data, indices and extreme events
http://flyingpigeon.readthedocs.io/en/latest/
Apache License 2.0
19 stars 15 forks source link

Migrate _handler to __init__.py #256

Closed Zeitsperre closed 5 years ago

Zeitsperre commented 6 years ago

https://github.com/bird-house/flyingpigeon/blob/647019ee2b59c01b43856d605318f74528e9ce06/flyingpigeon/processes/wps_subset_continents.py#L87

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 commented 6 years ago

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

Zeitsperre commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

cehbrecht commented 6 years ago

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 https://github.com/bird-house/cookiecutter-birdhouse/issues/3

cehbrecht commented 6 years ago

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

nilshempelmann commented 5 years ago

@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 commented 5 years ago

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

nilshempelmann commented 5 years ago

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

Zeitsperre commented 5 years ago

Closing this for now.