ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Option to sort re-exports in `__all__` (`sort_exports` from isort)

Open Jackenmen opened this issue 2 years ago • 4 comments

Isort allows sorting __all__ when sort_reexports is used: https://github.com/PyCQA/isort/blob/f547290178f86b6e5f0007cf1cb31d4736dcdaee/isort/core.py#L235-L241

Jackenmen avatar Dec 11 '22 18:12 Jackenmen

Is this undocumented behavior?

charliermarsh avatar Dec 11 '22 20:12 charliermarsh

isort has not had a release for over a year so it's one of the features that never ended up getting released. Actually, that reminds me of another (more important) feature that also has not been released yet, and seems like ruff isn't handling it properly either, will make an issue in a moment.

Jackenmen avatar Dec 11 '22 22:12 Jackenmen

Note that there's another functionality that is available in a released version of isort - sorting literals that are prepended with # isort: list or # isort: tuple: https://github.com/PyCQA/isort/issues/1358 sort_reexports option is actually using the same code for sorting.

One thing that both of these don't support is __all__ with multiple sections in it that should be sorted separately, e.g. something like what typing's code does (well, sort of, it doesn't seem like they're that good at keeping it alphabetical manually :smile:): https://github.com/python/cpython/blob/606adb4b891b52c8b9a53d29d594e996f117c0b3/Lib/typing.py#L42-L152 I'm actually not sure if isort's literal sorting even supports other profiles such as Black's, it seems to just sort it all in a pre-defined way.

Jackenmen avatar Dec 11 '22 22:12 Jackenmen

Oh yeah, this was brought to my attention on Twitter. I think we could support this! There are some other things I need to get done before I spend time on it though.

charliermarsh avatar Dec 11 '22 22:12 charliermarsh

I have a suggestion to extend this feature request a bit and allow sorting by the definition order of names (functions/classes/variables) that are put in __all__.

So for example:

from .submodule import reexported_name

__all__ = (
    "reexported_name",
    "function_b",
    "X",
    "function_a",
    "function_c",
    "Y",
)


def function_b():
    ...


class X:
   ...


def function_a():
    ...


def function_c():
    ...


class Y:
   ...

The reason for such a suggestion is that I think it makes it easier to find out what names of public APIs are missing in __all__ and what names shouldn't be in it (especially the latter one) since it allows one to just go through the file and look through the names in __all__ in the same order.

Jackenmen avatar Apr 15 '23 11:04 Jackenmen

For reference, there's another tool available to sort __all__: https://github.com/cpendery/asort

I guess it could be used to get rid of isort while we wait to have this feature implemented in ruff.

kdeldycke avatar Apr 15 '23 11:04 kdeldycke

For reference, there's another tool available to sort __all__: https://github.com/cpendery/asort

Hmm, forget about asort, I just stumble upon an issue: asort breaks docstring indention (asort#8)

That being said, regarding the sorting order, I also proposed to add support for another one that would be a case-insensitive lexicographical sorting. Not sure how popular or common this is.

kdeldycke avatar Apr 16 '23 05:04 kdeldycke

@cpendery - Yeah I'm open to including this. We'll need to decide how it should be categorized (i.e., does it go in the isort rule set? Or the Ruff-specific rules?) but it seems like a reasonable thing to include.

charliermarsh avatar Jul 24 '23 23:07 charliermarsh

@cpendery - Yeah I'm open to including this. We'll need to decide how it should be categorized (i.e., does it go in the isort rule set? Or the Ruff-specific rules?) but it seems like a reasonable thing to include.

@charliermarsh I think it probably should be categorized as Ruff specific rule since it was never an officially released isort feature, especially if we extend it with the additional functionality of multiple sorting options

cpendery avatar Jul 24 '23 23:07 cpendery

I have a suggestion to extend this feature request a bit and allow sorting by the definition order of names (functions/classes/variables) that are put in __all__.

This existed with isort's # isort: unique-list code styling comment as described in #7929.

kkirsche avatar Oct 12 '23 17:10 kkirsche

We've tried using isort's sort_exports in our project and it has several problems (from https://github.com/python-telegram-bot/python-telegram-bot/pull/4052#issuecomment-1875942280):

  • Doesn't honor black settings, i.e. uses single quotes (') instead of double (").
  • collapses multiline definitions (https://github.com/PyCQA/isort/issues/2193)

We found a new package https://pypi.org/project/sort-all/ which fixes these issues, however that package has one drawback of removing the comments in the __all__.

harshil21 avatar Jan 07 '24 17:01 harshil21

Adding to @harshil21 s comment, I'd like to mention that we're also interested in sorting __slots__ in addition to __all__.

Bibo-Joshi avatar Jan 08 '24 16:01 Bibo-Joshi

I'm working on a PR to implement this feature.

AlexWaygood avatar Jan 08 '24 16:01 AlexWaygood

One thing that both of these don't support is __all__ with multiple sections in it that should be sorted separately, e.g. something like what typing's code does (well, sort of, it doesn't seem like they're that good at keeping it alphabetical manually 😄): https://github.com/python/cpython/blob/606adb4b891b52c8b9a53d29d594e996f117c0b3/Lib/typing.py#L42-L152

So, a question on this: what should ruff do if it encounters something like this?

__all__ = [
    "d",
    "c",
    # a comment introducing a new section for 'b' and 'a':
    "b",
    "a",
]

Should it:

  1. Emit the violation on this snippet (since __all__ is not sorted), but refuse to autofix it, since the comment on its own line introduces an ambiguity regarding the user's intent

  2. Sort __all__ like this, assuming that the user wanted to use the comment to create a new section:

    __all__ = [
        "c",
        "d",
        # a comment introducing a new section for 'b' and 'a':
        "a",
        "b",
    ]
    
  3. Same as (2), but only if the user enabled a specific configuration flag on the command line or in their config file to enable "sorting per-section"?

I think the thing we probably shouldn't do in this kind of situation is this:

__all__ = [
    "a",
    "b",
    # a comment introducing a new section for 'b' and 'a':
    "c",
    "d",
]

AlexWaygood avatar Jan 10 '24 08:01 AlexWaygood

For alphabetical definitely option 2. For sort by definition order I'm not really sure what would work best, I guess anything is fine though I would definitely prefer if whatever it does didn't require a manual fix.

Jackenmen avatar Jan 10 '24 09:01 Jackenmen

This is an interesting question. I would expand the example by one that uses a non-section defining comment:

__all__ = [
	"c",
	# TODO: Remove once #0000 is complete
	"a"
]

In this case, I would expect the comment to move with a.

From what I understand is that isort requires the use of isort suppression comments to suppress import sorting. What if we would do the same for __all__?

__all__ = [
    "c",
    "d",
    # a comment introducing a new section for 'b' and 'a':
    "a",  #  noqa: RUFXXX Don't move 
    "b",
]

or

__all__ = [
    "c",
    "d",
    # a comment introducing a new section for 'b' and 'a':  #  noqa: RUFXXX Don't move
    "a",  
    "b",
]

MichaReiser avatar Jan 10 '24 09:01 MichaReiser

This is an interesting question. I would expand the example by one that uses a non-section defining comment [...] In this case, I would expect the comment to move with a.

Yes, that's exactly the kind of thing I was worrying about w.r.t. Option (2) -- thanks!

From what I understand is that isort requires the use of isort suppression comments to suppress import sorting. What if we would do the same for __all__?

That's an interesting idea! Let's call that Option (4). I worry with Option (4) that the precise semantics of the noqa directive might be hard for users to get their head around, though. You'd be applying a noqa directive to a specific line in __all__, but the addition of the noqa directive could have an impact on ruff's treatment of all items in __all__.

Here's three possible things the noqa directive could mean if it were applied to a comment at "index 3" in a multiline __all__ (I don't find (2) very plausible, but it would at least be simpler to implement than (3) 😅)

  1. The comment should remain at "index 3", but all items in __all__ are free to "move around" it.
  2. The comment should remain at "index 3". All items above it should be sorted, but all items below it should be untouched.
  3. The comment should remain at "index 3", and should be treated as a section delimiter. All items in the range of (previous_delimiting_comment_index + 1)..3 should be sorted within their section, and all items in the range of 4..index_of_previous_delimiting_comment should be sorted within their section.

I guess I'm leaning towards Option (3) in my original selection of options:

  • By default, comments move with the items below them with multiline __all__ definitions.
  • But, if a specific configuration flag is enabled, the behaviour changes so that ruff treats all comments on their own line as being "section delimiters" -- it will sort imports according to each section it spots, but won't disturb the sections.

If neither of the above is what the user wants, they can just # noqa the whole definition of __all__ (or just not opt into the rule at all!), so Option (1) in my original selection isn't very appealing here.

AlexWaygood avatar Jan 10 '24 10:01 AlexWaygood

For sort by definition order I'm not really sure what would work best

FWIW, my initial implementation of this rule is just going to do "sort by alphabetical order", so that I can get a MVP out there :)

We can possibly implement "sort by definition order" as a followup.

AlexWaygood avatar Jan 10 '24 10:01 AlexWaygood

That's an interesting idea! Let's call that Option (4). I worry with Option (4) that the precise semantics of the noqa directive might be hard for users to get their head around, though. You'd be applying a noqa directive to a specific line in all, but the addition of the noqa directive could have an impact on ruff's treatment of all items in all.

That's fair. Do you know how common the use of sections is? If they're uncommon, then disabling sorting for the entire __all__ might be a reasonable option.

Is there some sort of a community standard when it comes to formatting section comments, that we could use to identify them? If not, could we allow users to configure a regex to identify the section comments?

I guess I'm leaning towards Option (3) in my https://github.com/astral-sh/ruff/issues/1198#issuecomment-1884429395:

I fear that a project-level configuration might not be granular enough, but maybe it is. I really don't know.

FWIW, my initial implementation of this rule is just going to do "sort by alphabetical order", so that I can get a MVP out there :)

That's an excellent plan. Let's get sorting working and figure out how to disable sorting at a later stage :)

MichaReiser avatar Jan 10 '24 11:01 MichaReiser

Do you know how common the use of sections is? If they're uncommon, then disabling sorting for the entire __all__ might be a reasonable option.

I think they're reasonably common, but I don't really know -- I might just think this because I spend a fair amount of time staring at typing.py over at CPython, and, as pointed out earlier, we use them there :)

Is there some sort of a community standard when it comes to formatting section comments, that we could use to identify them?

I don't think so, not really :( Historically, there hasn't been a well-established tool for people to check the ordering of their __all__ definitions with, really, so I don't think it's really mattered in the past.

I fear that a project-level configuration might not be granular enough, but maybe it is. I really don't know.

Here's an idea: as well as simple noqa comments to switch isort on and off, isort also supports slightly more sophisticated "action comments": https://docs.astral.sh/ruff/linter/#action-comments. What if we had a version of that for this rule, as well as a global config setting? So, if ruff saw this:

__all__ = [  # ruff: sort-by-section
    ...
]

...then that would override the global config setting, and ruff would treat all comments taking up their own line in that __all__ definition as being section delimiters.

AlexWaygood avatar Jan 10 '24 11:01 AlexWaygood

Here's an idea: as well as simple noqa comments to switch isort on and off, isort also supports slightly more sophisticated "action comments": docs.astral.sh/ruff/linter/#action-comments. What if we had a version of that for this rule, as well as a global config setting? So, if ruff saw this

I'm a bit hesitant of adding more "magic comments" because they're hard to remember and not easily understood. You can't click on them and get an explanation of what they do; you need to know in which places they're allowed, and they don't work well with auto formatters (formatter needs to understand them as well). That's why I would favor a solution that doesn't require a new pragma comment.

MichaReiser avatar Jan 10 '24 12:01 MichaReiser

Okay, https://github.com/astral-sh/ruff/pull/9474 has now been merged! The next release of Ruff should include a rule and autofix to keep your __all__ definitions sorted. The sort order you get is an "isort-style" sort:

  • SCREAMING_SNAKE_CASE members come first
  • After that come CamelCase members
  • Anything else comes last
  • Within each casing category, members are sorted according to a natural sort.

Currently the rule is not configurable. We might consider adding some configuration options in the future, but for now we've decided to wait to see how much demand there is for that in the community.

The rule has been added as RUF022 rather than as an isort rule, since it seems like it doesn't really have much to do with imports. While isort does have this functionality, it's never been documented.

Enjoy tidying up your __all__ definitions! 😄

AlexWaygood avatar Jan 16 '24 15:01 AlexWaygood