polars icon indicating copy to clipboard operation
polars copied to clipboard

Proposal to improve ordering of methods in classes

Open stinodego opened this issue 2 years ago • 8 comments

Problem Description

Another attempt at keeping our huge classes somewhat manageable (and satisfying my OCD 😸).

We have many methods for our Series/DataFrame/LazyFrame/Expr classes. I propose we order them like this:

  1. __init__ -- Main constructor
  2. @classmethod methods -- Alternate constructors follow the main constructor
  3. @property methods -- Rather important and usually only a few methods
  4. Dunder methods -- Special kind of properties
  5. Public methods -- Main body of public functionality
  6. Namespaces (for Series/Expr) -- must come last due to str method clashing with mypy.

Private methods can be defined below whichever method(s) they are called by.

Within these groups, we can sort alphabetically (my preference), or loosely follow the groups defined in the documentation (e.g. Series).

Benefits:

  • Easier to find the method you're looking for.
  • Easier for newbies to explore the code.

I realize this will be hard to enforce and will probably get messed up again over time, but having this ordering in place will hopefully encourage contributors to follow the status quo. And it's a very small effort to get this done in the first place.

Comments welcome!

stinodego avatar Aug 29 '22 15:08 stinodego

not super familiar with the Python ecosystem, but JS has a linting rule that can enforce this. I wonder if there is a python equivalent.

universalmind303 avatar Aug 29 '22 16:08 universalmind303

This might help a bit (no alphabetical ordering as far as I can see): https://pypi.org/project/flake8-class-attributes-order/

ghuls avatar Aug 29 '22 17:08 ghuls

This might help a bit (no alphabetical ordering as far as I can see): https://pypi.org/project/flake8-class-attributes-order/

That looks like it does the job, indeed! Nice find. Let me try a PR to see how that would look...

stinodego avatar Aug 29 '22 17:08 stinodego

I think logical ordering (by what it does or how it is used) is more useful than alphabetical. Alphabetical order is arbitrary with respect to functionality. In any case, modern IDEs like vscode greatly improved discoverability and search.

slonik-az avatar Aug 29 '22 18:08 slonik-az

I think logical ordering (by what it does or how it is used) is more useful than alphabetical. Alphabetical order is arbitrary with respect to functionality. In any case, modern IDEs like vscode greatly improved discoverability and search.

The code is currently ordered logically (kind of). The 'problem' with this is that it is arbitrary, and you don't really know which logical set of methods lives where.

Alphabetical order also groups logically to a certain degree - for example, all rolling_X methods will be grouped, all arg_X methods will be grouped, etc.

stinodego avatar Aug 30 '22 00:08 stinodego

This might help a bit (no alphabetical ordering as far as I can see): https://pypi.org/project/flake8-class-attributes-order/

Update: Implemented this for Series: #4617

Not really sure about this. It's a bit counterintuitive to have to put the # noqa statement on the method BEFORE the namespaces (that violate the property order).

After thinking about this, I think it's worth going through the code once to make these improvements as a one time effort. Should only take me an hour or so - a few minutes for Ritchie to review. I don't think it's worth introducing a (not very mature) linter and possible developer friction in order to enforce this. The gain isn't worth it.

stinodego avatar Aug 30 '22 00:08 stinodego

I had to break the proposed order in #4650 recently. Please see 4dcb09d42acdbbf09e07b94a0004ab56df2f5a7a.

Is my case special enough to break the rules?

abalkin avatar Sep 01 '22 04:09 abalkin

I had to break the proposed order in #4650 recently. Please see 4dcb09d.

Is my case special enough to break the rules?

For now, there are no rules. Don't worry about it 😉

Also, I think you don't have to break the rules. I'll make a suggestion.

stinodego avatar Sep 01 '22 04:09 stinodego