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

Optional lexicographic ordering #26

Open
AnonymouX47 opened this issue Feb 14, 2022 · 12 comments
Open

Optional lexicographic ordering #26

AnonymouX47 opened this issue Feb 14, 2022 · 12 comments
Labels
enhancement New feature or request help wanted Extra attention is needed open challenge Open to newcomers, but expected to be extremely difficult

Comments

@AnonymouX47
Copy link

Topological ordering is good for understanding code but becomes less effective as the module becomes larger and dependency becomes more complex.

In such cases (and even in small modules), I find lexicographic (cos identifiers might also include digits) ordering much more fitting.

@AnonymouX47
Copy link
Author

AnonymouX47 commented Feb 14, 2022

It'll be more like "restricted lexicographic ordering" because of cases like decorators... In such cases, topological order overrides i.e for decorators, their dependencies and their dependants (decorated definitions).

Maybe:

  • decorators (with their dependencies) can be grouped before other definitions, OR
  • definitions decorated with decorators defined within the same module are grouped after other definitions.

@bwhmather
Copy link
Owner

bwhmather commented Feb 14, 2022

I'm not sure. The problem is sorting lexicographically prevents grouping. Things like this (names carefully chosen to sort alphabetically) will be common:

def internal_dependency():
    pass
    
def something_unrelated():
    pass
    
def top_level()
    return internal_dependency()

One thing I experimented with early on was trying to infer groupings. My approach boiled down to doing a DAG traversal, either breadth first or depth first. Unfortunately, both breadth first and depth first orders seemed to lead to different pathological behaviours. I couldn't think of a more intelligent way to do it so I dropped the idea. My attempt sorted dependencies by the order in which they were called by a function, but it's possible lexicographical order would make things a bit more predictable.

@AnonymouX47
Copy link
Author

AnonymouX47 commented Feb 14, 2022

I really don't see how it would affect grouping 🤔... Grouping by type (classes, functions, etc... for top-level statement and the already existing grouping for class members) but taking the case of decorators into consideration, since they're executed immediately after the decorated definition.

I fail to see how the example above conflicts with lexicographic ordering...

Also, the README says "leaves grouping to the programmer", what exactly does that mean?

@bwhmather
Copy link
Owner

By grouping, I mean the sort of manual grouping where you cluster related functions closer to each other. In the example I gave above, the natural grouping would be something like this:

# === Group 1 ====
def something_unrelated():
    pass
  
# === Group 2 ===
def internal_dependency():
    pass
      
def top_level():
    return internal_dependency()

ssort currently only enforces that internal_dependency must be ahead of top_level. So long as this is done, the programmer is free to manually arrange statements however they like.

@AnonymouX47
Copy link
Author

AnonymouX47 commented Feb 15, 2022

I see... then such grouping obviously won't work with lexicographic ordering.

A workable style of grouping I can think of is the one i mentioned earlier... grouping by "type/category" (with decorators and decorated definitions as an exception) e.g:

At the top level

  • classes
  • functions: Optionally sub-grouped into public and private
  • data items:
    • constants (Optionally sub-grouped into public and private)
    • variables (Optionally sub-grouped into public and private)
  • etc...

* private refers to names starting with an underscore.

Any definition or data item require before another has to come before it :( ...

Any definition decorated with another callable defined within the same module, and it's decorator must be treated as a special case.
If any top-level definition is used as a decorator within a class definition, it has to come before the class definition.

Within classes

  • __slots__
  • Class data attributes
    • Public
    • Private
    • Mangled
  • Special methods
    • Instantiation-related come first e.g __init__, __new__.
    • Others come after in alphabetical order.
  • Properties
  • Methods
  • Public methods
  • "Private" methods (Starting with single underscore)
  • Mangled-name methods (Starting with double underscore)
  • etc...

Any definition decorated with another callable defined within the same class, and it's decorator must be treated as a special case.


As for decorators and decorated definitions, they could probably be a separate group within which the already established topological ordering is applied.


As seen, there'll definitely be need for topological ordering in some cases... to solve this, i think first sorting lexicographically and grouping as above, while ignoring dependency, and then taking the result through a topological sort should work.

The END

Here's my proposition.

@AnonymouX47
Copy link
Author

AnonymouX47 commented Feb 15, 2022

Do you see it feasible?

@bwhmather
Copy link
Owner

Definitely feasible. Grouping by type is basically what we are doing for classes at the moment, although I'm not completely satisfied with exactly how we order the groups. I have an open issue (#11) to discuss that, and have copied and pasted your suggestion over there.

I think it's more tricky to get right at the module level as modules have fewer types of attributes (few d'under methods, no properties, classmethods, etc), and those attributes have more, and stronger, interdependencies (i.e. not via self).

I think the answer is to get class body sorting nailed down first, and then come back to this with what we've learned before making the call on whether to cut 1.0.

@AnonymouX47
Copy link
Author

Hmm 🤔... you're right.

Let's first see how it goes with classes then.

@AnonymouX47
Copy link
Author

Thinking about the lexicographic ordering again...

The only way I see it working for all cases is in this order:

  • Sort lexicographically.
  • Group
  • Sort topologically.

If the custom grouping I suggested in #11 is implemented, then it should be able to reduce the distortion of grouping caused by topological sorting if the user will group such related statements together with a custom group.

@AnonymouX47
Copy link
Author

I'm really interested in the project and would like to be more involved and get into implementation but at the moment the best I can do is make suggestions as I'm currently really engrossed in another project.

@bwhmather
Copy link
Owner

Yup, that distortion bit is key. We do distinguish between soft ("deferred") and hard requirements when we first scan the file. I think it should be possible to turn off topological sorting based on soft dependencies when grouping or lexicographical sorting is applied. I think we could go a fair way towards unifying the handling of module and class bodies with this approach.

Unfortunately this is, likewise, a bit more than I can chew at the moment. Don't worry about not being able to get stuck in to implementation as conversations like this are essential to keeping the project on track and more than help enough.

@AnonymouX47
Copy link
Author

Ok, that's good.

@bwhmather bwhmather added enhancement New feature or request help wanted Extra attention is needed open challenge Open to newcomers, but expected to be extremely difficult labels Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed open challenge Open to newcomers, but expected to be extremely difficult
Projects
None yet
Development

No branches or pull requests

2 participants