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

[WIP] Editing around comments #14886

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Dec 10, 2024

This is an attempt to introduce some helper functions for making edits without removing comments. The plan is to implement the new helper functions in a number of different rules, and see if the ecosystem check raises any alarms. If the approach starts to look reasonable, I will break this into separate PRs.


In case anyone is interested, the rough idea is as follows:

  1. First, handle deletions. If you are making a deletion edit in some range [s0, e0] and there are comment ranges in this interval, say [s1,e1], [s2,e2], ..., [sn,en], then you could instead just delete each of the ranges [s0,s1], [e1,s2], ..., [en,e0]. There is some bookkeeping that needs to be done, however, around newlines and indentation (and probably more things I haven't thought of!), but that is the general idea.
  2. Insertions are already okay.
  3. Any range replacement can be written as a deletion following by an insertion. So use (1) and then insert. The downside of this approach is that it pushes all the comments to the top of the replacement. But it seems like this may be slightly better than deleting them.

I think a more sophisticated approach can be developed on top of the above by first creating a diff that breaks up the range replacement into several replacements, and then applies (3) to each piece. But one thing at a time...

Finally, you probably shouldn't look at the implementation details at the moment - the code itself is ugly and probably not performant or memory-conscious, I'm just trying to get something logically correct first and then I'll try to make it pretty.

This may all be terribly misguided... we'll see...

@dylwil3 dylwil3 added the do-not-merge Do not merge this pull request label Dec 10, 2024
Copy link
Contributor

github-actions bot commented Dec 10, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@AlexWaygood AlexWaygood added the fixes Related to suggested fixes for violations label Dec 10, 2024
Comment on lines -270 to +279
Unsafe fix
Safe fix
43 43 | MyType = TypedDict("MyType", dict())
44 44 |
45 45 | # Unsafe fix if comments are present
46 |-X = TypedDict("X", {
47 |- "some_config": int, # important
48 |-})
46 |+class X(TypedDict):
47 |+ some_config: int
46 |+# important
47 |+class X(TypedDict):
48 |+ some_config: int
Copy link
Member

@AlexWaygood AlexWaygood Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic improvement, and we shouldn't let the perfect be the enemy of the good, so I'd happily merge a change to our fix infrastructure that preserved comments like this. However... I'd still mark this fix as unsafe if it moved comments like this. It would be very annoying if you e.g. had a TypedDict like this:

Config = TypedDict("Config", {
    "x": int,  # this is the x field. It's an int because numbers are great.
    "y": str,  # this is the y field. It's a string because some things are string.
    "z": bytes,  # this is the z field. TODO: why is it bytes??
})

And Ruff then autofixed it to this:

# this is the x field. It's an int because numbers are great.
# this is the y field. It's a string because some things are string.
# this is the z field. TODO: why is it bytes??
class Config(TypedDict):
    x: int
    y: str
    z: bytes

One way to think about it is: would you be happy for Ruff to automatically apply these fixes every time you pressed save in an editor, like you might with an autoformatter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Do not merge this pull request fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants