[flake8-pyi] Mark PYI030 fix unsafe when comments are deleted

[VascoSch92]

The PR addresses issue #16317

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[ntBre]

Thanks for taking care of this so quickly! I have minor nits about the code and docs and one larger question about the number of tests, which I have not reviewed fully yet.

[ntBre]

Could we just return the Edit from both branches of the if other_exprs.is_empty()? Then we could avoid duplicating the comment check. I'm picturing something like this truncated example:

let edit = if other_exprs.is_empty() {
    Edit::range_replacement(checker.generator().expr(&literal), expr.range())
} else {
    // ...
    Edit::range_replacement(content, expr.range())
};

if checker.comment_ranges().intersects(expr.range()) {
    Fix::unsafe_edit(edit)
 } else {
     Fix::safe_edit(edit)
}

The code was already structured like this before your changes, but I also find it a bit weird to have this huge block inside of the diagnostic.set_fix call. That makes sense when using Diagnostic::try_set_fix with a closure but not so much here. For the sake of an easy-to-review diff, we might want to leave that alone for now, though.

[VascoSch92]

It makes sense. I changed ;-)

[ntBre]

I know I linked to TC006, which has this kind of phrasing, but I would probably phrase this more like "This fix is marked unsafe if it would delete any comments within the replacement range."

And if you want to go above and beyond you can include an example like SIM905. Maybe something like

field26: (
    # preserved comment
        Literal["a", "b"]
        # deleted comment
        | Literal["c", "d"]  # preserved again
)

(assuming I've identified where comments are preserved and deleted correctly)

[ntBre]

It feels wrong for me to complain about having too many tests, but are any of these cases addressing particular subtleties here? They all look roughly the same to me, and I think they are all unsafe fixes? Maybe it would be more helpful if each comment were something like "ok" or "preserved" for any comments we preserve and "deleted" or "not ok" for deleted comments.

Either that or trimming down the sheer number of cases would make this a little easier to review, at least for me.

[AlexWaygood]

I agree with Brent -- I think just this first snippet would probably be sufficient to test this PR :-)

This kind of thing is something we do in lots of rules, so I think we can be pretty confident that this patch works without testing every single edge case in this rule's fixtures. And having too many tests can be confusing for future readers of the fixture -- the reader could be left thinking that the rule is more complex than they thought it was, or they could be left wondering why there are so many snippets when they all seem to be testing the same thing.

[VascoSch92]

Ok, no problem. I just left the first two test-cases. I hope it is enough. Let me know

[VascoSch92]

About the CI/mkdocs failing: should I had a KNOWN_FORMATTING_VIOLATIONS ?

[AlexWaygood]

Thank you! I fixed the docs formatting issue :-) It was complaining that you didn't have enough spaces before an inline comment