Skip to content

DiagnosticVerifier: Allow specifying locations vs. ranges in fix-it verifications #41484

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

Closed
wants to merge 2 commits into from

Conversation

AnthonyLatsis
Copy link
Collaborator

This spares us from needless repetition in the bounds of insertion fix-its.

@AnthonyLatsis AnthonyLatsis changed the title DiagnosticVerifier: Allow specifying locations in fix-it verifications DiagnosticVerifier: Allow specifying locations vs. ranges in fix-it verifications Feb 20, 2022
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test macOS

@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test macOS

@rintaro
Copy link
Member

rintaro commented Feb 21, 2022

Sorry, but I personally don't think this is a good idea. {{5=a}} looks like replacing the character at column 5 with a at a glance.
Are there particular cases we strongly want this change?

@AnthonyLatsis
Copy link
Collaborator Author

AnthonyLatsis commented Feb 22, 2022

It reduces duplication, and therefore potential typing errors, and makes it easier to read through numerous verified fix-its that benefit from this change (#41198 has an example test file), especially the ones that use offsets like -1:1--1:1 (I prefer these for robustness). It sounds like an edge case for someone to mistake 5-6= for 5=. We could continue rendering a range to clarify in case that happens. WDYT?

@rintaro
Copy link
Member

rintaro commented Feb 22, 2022

For making fix-it test cases, I usually add {{none}} to expected-error etc. so the verifier can suggest the "correct" expected fix-its. So I rarely type expected fix-its manually. (Of course, I always check the suggested fix-its are actually correct.)
As for readability, for me, since they are test cases, I'd more appreciate if all test cases are written in a consistent syntax.

@AnthonyLatsis
Copy link
Collaborator Author

{{none}} is convenient, but even if you use --verify-apply-fixits, you still have to add lines/offsets on your own. On the other hand, the force-verifying of lines that we discussed earlier would alleviate the latter to some extent. Anyway, thanks for the feedback!

@AnthonyLatsis AnthonyLatsis deleted the fixit-locs branch February 22, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants