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

RUF022 autofix should be marked unsafe if there are own-line comments between __all__ items #14552

Closed
phil65 opened this issue Nov 23, 2024 · 4 comments · Fixed by #14560
Closed
Assignees
Labels
fixes Related to suggested fixes for violations

Comments

@phil65
Copy link

phil65 commented Nov 23, 2024

  • I searched issues for __all__, couldnt see anything from today.

Since 0.8.0, ruff wants to sort my __all__ array.
Thats generally very nice, but it also messes up something like.

__all__ = [
    # Core components
    "TaskManager",
    "TaskExecutor",
    # Models
    "TaskContext",
    "TaskProvider",
    "TaskResult",
    # Utilities
    "execute_concurrent",
]

gets sorted to:

__all__ = [
    # Models
    "TaskContext",
    "TaskExecutor",
    # Core components
    "TaskManager",
    "TaskProvider",
    "TaskResult",
    # Utilities
    "execute_concurrent",
]

So it messes up the groups.
Comments are only attached to one item instead of the group when sorting. I am not sure if that is a good assumption. Perhaps ignore sorting it when comments are inside the all array?

Thanks! :)

@MichaReiser
Copy link
Member

MichaReiser commented Nov 23, 2024

Thanks for opening this issue.

Comments are difficult to get right because they have no semantic meaning. Is # Core components commenting TaskManager or the entire group? I also know that @AlexWaygood spent a lot of time adding good comment support. So I'm interested to hear his thoughts too.

I see two possible changes instead of removing the fix entirely:

  1. Mark the fix as unsafe if there are own-line comments because the fix might change the comment's semantic
  2. Improve the heuristic. For example, the rule could implement sort-by-group similar to isort if some items are separated by blank lines
__all__ = [
    # Core components
    "TaskManager",
    "TaskExecutor",

    # Models
    "TaskContext",
    "TaskProvider",
    "TaskResult",
    
    # Utilities
    "execute_concurrent",
]

Is formatted to

__all__ = [
    # Core components
    "TaskExecutor",
    "TaskManager",

    # Models
    "TaskContext",
    "TaskProvider",
    "TaskResult",
    
    # Utilities
    "execute_concurrent",
]

I'm sure @AlexWaygood has some thought

@MichaReiser MichaReiser reopened this Nov 23, 2024
@MichaReiser MichaReiser added rule Implementing or modifying a lint rule fixes Related to suggested fixes for violations and removed rule Implementing or modifying a lint rule labels Nov 23, 2024
@AlexWaygood
Copy link
Member

Thanks for opening the issue @phil65!

There's already some extensive documentation on the fix safety of this rule at https://docs.astral.sh/ruff/rules/unsorted-dunder-all/#fix-safety, and we note this problem there. However, I'm open to making this fix unsafe if there are any own-line comments. I think that's in the spirit of the policy on fix safety we recently adopted when it comes to comments. We should make the same change to sort-dunder-slots as well.

2. Improve the heuristic. For example, the rule could implement sort-by-group similar to isort if some items are separated by blank lines

Unfortunately doing this would make the rule incompatible with the formatter, because the formatter will remove blank lines in between the items, which would then lead to the rule applying a different sort to the items than if the blank lines were present.

@MichaReiser
Copy link
Member

Unfortunately doing this would make the rule incompatible with the formatter, because the formatter will remove blank lines in between the items, which would then lead to the rule applying a different sort to the items than if the blank lines were present.

Right, I think we even discussed this back then. Thanks for reminding me.

@AlexWaygood AlexWaygood changed the title Over-eager __all__ sorting RUF022 autoroute shouldOver-eager __all__ sorting Nov 23, 2024
@AlexWaygood AlexWaygood changed the title RUF022 autoroute shouldOver-eager __all__ sorting RUF022 autofix should be marked unsafe if there are own-line comments Nov 23, 2024
@AlexWaygood AlexWaygood changed the title RUF022 autofix should be marked unsafe if there are own-line comments RUF022 autofix should be marked unsafe if there are own-line comments between __all__ items Nov 23, 2024
@AlexWaygood AlexWaygood self-assigned this Nov 23, 2024
@phil65
Copy link
Author

phil65 commented Nov 23, 2024

Thank you for quick handling and a great piece of software. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
3 participants