LINT: few remaining refactor and warning flags to be resolved

[fkuehlein]

Working on getting the Travis build pass again, I resolved many minor flake8 and pylint flags in b9a31aad0e9f3f4704af523b4947d311dacc802f and 42734ff67c12854b898b2e334c372e4e46777775.

flake8 is passing now, but there are some pylint flags remaining that I wasn't yet able to resolve on my own. @ntfrgl, could you give me some advice on those? I know there's other open issues right now, but I think getting the Travis build to pass again would be very useful for all subsequent work and it does not seem to require much more.

In order of ascending difficulty (to me):

1. too many arguments in method

   in climate.EventSeriesClimateNetwork.__init__() pylint says:

   src/pyunicorn/climate/eventseries_climatenetwork.py:47:4: R0913: Too many arguments (16/12) (too-many-arguments)

   Any downsides of configuring pylint to allow for 16 arguments instead of 12?

2. try: return; except Error:-clauses

   in e.g. core.Grid.Load() pylint says:

   src/pyunicorn/core/grid.py:124:4: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)

   Put the return statement after the try-except instead? (Or don't re-raise in place at all, as is advised in this SO-thread? Would then lose the additional info though ..)

3. arguments reordered in overriding methods

   in several places (see below), pylint says e.g:

   src/pyunicorn/core/geo_grid.py:175:4: W0237: Parameter 'space_grid' has been renamed to 'lat_grid' in overriding 'GeoGrid.RegularGrid' method (arguments-renamed)

   In some cases, arguments are actually just renamed, which is resolvable. In others, this is due to additional required arguments in the overriding method in question, which have to stand before the optional arguments. This causes a reordering of arguments. But apparently, you can have parameters in the child class which don't feature in the parent class, so long as they are optional!

   This concerns

   • timeseries.JointRecurrenceNetwork.set_fixed_recurrence_rate() (overriding the parent class method timeseries.JointRecurrencePlot.set_fixed_recurrence_rate())
     • density instead of recurrence_rate
   • core.SpatialNetwork.save() (overriding the parent class method core.Network.save())
     • filename_network instead of filename
   • core.GeoGrid.RegularGrid() (overriding the parent class method core.Grid.RegularGrid())
     • lat_grid, lon_grid instead of space_grid
   • core.SpatialNetwork.Load() (overriding the parent class method core.Network.Load())
     • filename_network, filename_grid instead of filename
   • climate.ClimateNetwork.Load() (overriding the parent class method core.GeoNetwork.Load())
     • filename_similarity_measure added before optional args
   • climate.ClimateData.Load() (overriding the parent class method core.Data.Load())
     • time_cycle, time_name="time", latitude_name="lat", longitude_name="lon", data_source=None, file_type="NetCDF" instead of file_type, dimension_names=None

   The first two should be easy fixes (rename), maybe also the following two (put two arguments into one). The last two, however, especially the very last one, seem not so easy to resolve. Any idea of a fix that does not require major restructuring? Or ignore them?

Thanks in advance!

---

See full pylint report here: https://gist.github.com/fkuehlein/fbc67b94a9425544857a612a3b15d0d3 It contains some further 'convention' flags, but I'll treat those separately.

[ntfrgl]

Thank you for this cleanup!

Most of these issues have accumulated over the past few years, during which testing and linting were apparently not used in development even locally. An exception is utils.navigator, which I had excluded from the cleanup before the initial public release. The configuration update in 911fb11 unfortunately missed moving the ignore pattern from pytest-flake8 to flake8, but I have now removed it entirely in aa56ad1.

Regarding your questions:

1. Note that the default threshold is 5, and I had originally raised it to 12, which was a compromise between refactoring effort and linting benefit. Further raising this value to 16 would effectively switch off the warning. I would rather suggest grouping related arguments into dictionaries, e.g., in this case by the parent classes they are passed to.

2. This sort of encoding choice is contextual. In principle, you could have an explicit return None or some other representation of failure along the exception-handling branch, but note that the underlying cause for the linter warning here is that the exception is not being re-raised. In this specific case I agree with the SO answer you linked, to the extent that it argues against internally "wrapping" the exception with print statements --- The print statement in the catch clause is currently not only failing to add any useful information beyond what would already be contained in the exception, but by successfully returning an invalid object (None) it requires explicit downstream failure detection.

3. All of these class inheritance issues arise from a lack of precision in the abstractions encoded by the classes.

   • I agree with your suggestions about the cases 1-3, with the addition of checks ensuring that the "optional" argument filename_grid is present and that the argument (lat_grid,lon_grid) ~ space_grid, which should be treated uniformly across all relevant classes, has the correct shape.
   • For the cases 4-6, I would similarly make the newly added argument(s) optional, but asserting their presence in the method body, and realigning the treatment of dimension_names in ClimateData.Load() with that in Data.Load().

Please also make sure to update the tests and tutorials accordingly.

[fkuehlein]

Thank you for the advice and for updating the flake8 configuration!

I'll take care of those remaining fixes as soon as possible, hopefully later this week.