Follow up to "Unrelated comment was deleted after a change #495"

[BonaFideIT]

Python Version

3.11.10

Django Version

4.2.15

Package Version

1.22.1

Description

This is a follow up issue from #495.

As mentioned in pyupgrade in asottile/pyupgrade#931, the trailing comments problem is not fixable.

The following test case will fail pass:

def test_removed_block_internal_comment_2():
    check_transformed(
        """\
        import django

        if django.VERSION < (3, 2):
            foo()
            # test comment
        # test comment 2
        """,
        """\
        import django

        # test comment 2
        """,
    )

Should the changes from c219454 be reverted since it does not resolve all possible variants of the problem, @adamchainz what do you think?

[adamchainz]

I want to find the time to noodle about this one a bit first before acting. Perhaps there is a solution that we've all missed. I don't think the behaviour affects many cases either way so should be fine to leave as-is for a little bit.

[BonaFideIT]

Thanks! If we can help you with this, please do not hesitate to write.

[adamchainz]

Well, if you want, learn about tokenization and why the indentation doesn't tell you about comments. Because without looking deeper, it seems to me that the information should be retrievable, since tokenize-rt is able to roundtrip tokens.

Check out https://www.youtube.com/watch?v=G1omxo5pphw for Anthony's intro to using tokens and AST for formatters, how djanog-upgrade and pyupgrade work.

[BonaFideIT]

Ok, I watched the video and I dig into it a bit. I'm wrong your fix works fine, even for the test I mentioned above. So I'm not sure why this would be a problem as I haven't found an edge case where the test won't pass.

[adamchainz]

Cool. Maybe there are some tests we could add, at least? :)

[BonaFideIT]

If I understand the problem from the ticket here asottile/pyupgrade#931 correctly, removing can lead to invalid syntax. But as far as I can see, these cases cannot be solved automatically anyway. Moreover, they do not apply exactly to this case here.

But it's a good idea to add a few more tests to make sure that everything continues to work in the future. I will open a PR for some tests that might be useful.