xdem icon indicating copy to clipboard operation
xdem copied to clipboard

Add `ssort` to the pre-commit CI

Open erikmannerfelt opened this issue 10 months ago • 2 comments

ssort is a really cool tool that sorts functions and classes in the order that they are used. Basically, every file after sorting goes from low to high complexity down in the file. This makes it much easier to understand and debug the code.

A common problem would be that multiple PRs modify a file so the order of logic becomes weird:

def do_something():
  ...

class DEM:
  def func():
    _subfunction()


def _subfunction():
   do_something()

This is easily missed and gradually leads to more and more complex up-down-up-down logic. In the example above, ssort would move _subfunction to between the do_something function and the DEM class. Thus, if someone tries to find the definition of a function, one would only need to look up, not down and then up.

As with the big CI PRs, it's best to do this when no PR is active, as it will reorder a lot and thus make for really annoying PR merge conflicts.

What do you think of this addition, @rhugonnet @adehecq ? A similar PR should be done on geoutils.

erikmannerfelt avatar Aug 21 '23 11:08 erikmannerfelt

Yes, this seems great! I think we've tried to respect this most of the time, but enforced it would also make it become "obvious" when we need to split a big module into smaller modules (for example if a big bunch of functions start stacking above some classes).

Sometimes it's useful to have a "custom" order that one is used to. It might break some of our "section comments" we have in some modules, but maybe that'd be a cue to have those sections as submodules. Curious how it'll behave with classes (it says Groups class members by type and enforces topological sorting of methods., which sounds good!).

rhugonnet avatar Aug 21 '23 19:08 rhugonnet

Yes, I think we should first run it on our current code and see what the output looks like and whether or not it makes sense!

adehecq avatar Sep 06 '23 15:09 adehecq