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

Implement ssort #3946

Open
JackAshwell11 opened this issue Apr 12, 2023 · 10 comments
Open

Implement ssort #3946

JackAshwell11 opened this issue Apr 12, 2023 · 10 comments
Labels
accepted Ready for implementation plugin Implementing a known but unsupported plugin

Comments

@JackAshwell11
Copy link

Since Ruff already implements isort, it would be nice to see ssort implemented as well.

This plugin automatically reorders the contents of a python file so that statements are placed after the things they depend making navigation easier within the file. For example (from the ssort GitHub page):
Before:

from module import BaseClass

def function():
    return _dependency()

def _decorator(fn):
    return fn

@_decorator
def _dependency():
    return Class()

class Class(BaseClass):
    def public_method(self):
        return self

    def __init__(self):
        pass

After:

from module import BaseClass

class Class(BaseClass):
    def __init__(self):
        pass

    def public_method(self):
        return self

    def _decorator(fn):
        return fn

@_decorator
def _dependency():
    return Class()

def function():
    return _dependency()
@MichaReiser
Copy link
Member

Does ssort solve a similar problem to the newspaper rule #2436 (except that it fixes it automatically)?

@JackAshwell11
Copy link
Author

JackAshwell11 commented Apr 12, 2023

I haven't heard of flake8-newspaper-style before, but based on what I can gather, it seems so.

@g-as
Copy link
Contributor

g-as commented Apr 12, 2023

It seems that they do solve a similar problem but with opposite solutions.

ssort does bottom-up while flake8-newspaper-style does top-down.

https://github.com/bwhmather/ssort#frequently-asked-questions

@JackAshwell11
Copy link
Author

JackAshwell11 commented Apr 12, 2023

There is an active issue in ssort asking for more sorting options. This could be part of tool.ruff.ssort.sorting-order. Although, I'm not sure if top-down is the correct order for a scripting language such as Python since lookups happen immediately as described here.

@charliermarsh charliermarsh added the plugin Implementing a known but unsupported plugin label Apr 13, 2023
@tjkuson
Copy link
Contributor

tjkuson commented May 11, 2023

I would add that ssort conflicts with the Django documentation and the DJ012 rule. If ssort were to be implemented in ruff, I would love it if there were a sorting option to cohere with the Django standards (beyond just top-down vs bottom-up).

@JackAshwell11
Copy link
Author

I would add that ssort conflicts with the Django documentation and the DJ012 rule. If ssort were to be implemented in ruff, I would love it if there were a sorting option to cohere with the Django standards (beyond just top-down vs bottom-up).

There are some issues on ssort's GitHub page asking for options to change the sorting order. So that could work.

@JackAshwell11
Copy link
Author

I would like to have a go at implementing this, not sure how successful I'll be, but I can try. Just would like to finish my exams first.

@JackAshwell11
Copy link
Author

So after looking and understanding ssort's implementation, it definitely looks like it would be better to just re-write it entirely from the ground up. I'm gonna work on some other rules so I can get a bit more practice with Ruff's codebase first.

@jgberry
Copy link
Contributor

jgberry commented Jun 9, 2023

Hey @Aspect1103! I'm a ssort collaborator and have recently really taken an interest in ruff. I think there's a lot of promise here, specifically from a performance perspective. We spent considerable effort optimizing ssort's performance, but pure python can only take you so far. I could easily see a pure rust implementation having orders of magnitude better performance! Let me know if I can help out in any way!

@JackAshwell11
Copy link
Author

Hey @Aspect1103! I'm a ssort collaborator and have recently really taken an interest in ruff. I think there's a lot of promise here, specifically from a performance perspective. We spent considerable effort optimizing ssort's performance, but pure python can only take you so far. I could easily see a pure rust implementation having orders of magnitude better performance! Let me know if I can help out in any way!

Hi, I haven't yet had a go with making some Ruff plugins, so you could take this one on if you want.

charliermarsh pushed a commit that referenced this issue Jun 21, 2023
## Summary

According to the AST visitor documentation, the AST visitor "visits all
nodes in the AST recursively in evaluation-order". However, the current
traversal fails to meet this specification in a few places.

### Function traversal

```python
order = []
@(order.append("decorator") or (lambda x: x))
def f(
    posonly: order.append("posonly annotation") = order.append("posonly default"),
    /,
    arg: order.append("arg annotation") = order.append("arg default"),
    *args: order.append("vararg annotation"),
    kwarg: order.append("kwarg annotation") = order.append("kwarg default"),
    **kwargs: order.append("kwarg annotation")
) -> order.append("return annotation"):
    pass
print(order)
```

Executing the above snippet using CPython 3.10.6 prints the following
result (formatted for readability):
```python
[
    'decorator',
    'posonly default',
    'arg default',
    'kwarg default',
    'arg annotation',
    'posonly annotation',
    'vararg annotation',
    'kwarg annotation',
    'kwarg annotation',
    'return annotation',
]
```

Here we can see that decorators are evaluated first, followed by
argument defaults, and annotations are last. The current traversal of a
function's AST does not align with this order.

### Annotated assignment traversal
```python
order = []
x: order.append("annotation") = order.append("expression")
print(order)
```

Executing the above snippet using CPython 3.10.6 prints the following
result:
```python
['expression', 'annotation']
```

Here we can see that an annotated assignments annotation gets evaluated
after the assignment's expression. The current traversal of an annotated
assignment's AST does not align with this order.

## Why?

I'm slowly working on #3946 and porting over some of the logic and tests
from ssort. ssort is very sensitive to AST traversal order, so ensuring
the utmost correctness here is important.

## Test Plan

There doesn't seem to be existing tests for the AST visitor, so I didn't
bother adding tests for these very subtle changes. However, this
behavior will be captured in the tests for the PR which addresses #3946.
@jgberry jgberry mentioned this issue Jun 21, 2023
@charliermarsh charliermarsh added the accepted Ready for implementation label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation plugin Implementing a known but unsupported plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants