Use RewriteResult in visit_item #6410
Conversation
| return None; | ||
| return Err(RewriteError::Unknown); |
There was a problem hiding this comment.
Not something we need to do in this PR, but it would be nice to add a new variant to RewriteError that communicates that rewriting failed because comments would be lost.
There was a problem hiding this comment.
I was wondering about this too. Or maybe something more general like KnownLimitation. But would such an error actually be useful over Unknown at runtime?
There was a problem hiding this comment.
For the potential comment loss, yes it would be. Eventually we'll want to communicate to end users that we failed to format their code because we weren't expecting to find a comment in a particular location, and in order to avoid removing the comment entirely we're leaving that AST node unformatted.
There was a problem hiding this comment.
In some cases we already communicate that to end users, but it's not always consistent.
There was a problem hiding this comment.
Overall code changes look good to me. Should be good to merge after the Diff Check Job completes.
Edit: The job ran successfully ✅
CC #6206