[M] Change in dart/sdk[main]: [analysis_server] Don't let format errors fail change building
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
@FailingTest(
reason:
'This test relies on the formatter failing to return the error, but '
'ChangeBuilder now guards against formatter errors. If this code is '
'in use, we should change the handler to parse the resulting content '
'and check whether there are parse errors, instead of using the '
'formatter for this',
)This relates to my comment at https://github.com/dart-lang/sdk/issues/62866#issuecomment-4054701940
I think we could fix this (by having this handler check for parse errors in a copy of the new file with the edits applied), however I think this might be completely unused code from a previous editor experiment, so wonder if it's worth it?
(cc @pquit...@google.com @hel...@google.com in case my search wasn't complete and you think `flutter.setWidgetPropertyValue` might be used by IntelliJ)
Related details
- Brian Wilkerson
- Helin Shiah
- Phil Quitslund
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Phil Quitslund (Gerrit)
Phil Quitslund added 1 comment![]( "Open in Gerrit")
@FailingTest(
reason:
'This test relies on the formatter failing to return the error, but '
'ChangeBuilder now guards against formatter errors. If this code is '
'in use, we should change the handler to parse the resulting content '
'and check whether there are parse errors, instead of using the '
'formatter for this',
)This relates to my comment at https://github.com/dart-lang/sdk/issues/62866#issuecomment-4054701940
I think we could fix this (by having this handler check for parse errors in a copy of the new file with the edits applied), however I think this might be completely unused code from a previous editor experiment, so wonder if it's worth it?
(cc @pquit...@google.com @hel...@google.com in case my search wasn't complete and you think `flutter.setWidgetPropertyValue` might be used by IntelliJ)
Thanks for looping me in. I don't see any uses in the plugins either.
Related details
- Brian Wilkerson
- Danny Tuppeny
- Helin Shiah
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
@FailingTest(
reason:
'This test relies on the formatter failing to return the error, but '
'ChangeBuilder now guards against formatter errors. If this code is '
'in use, we should change the handler to parse the resulting content '
'and check whether there are parse errors, instead of using the '
'formatter for this',
)Phil QuitslundThis relates to my comment at https://github.com/dart-lang/sdk/issues/62866#issuecomment-4054701940
I think we could fix this (by having this handler check for parse errors in a copy of the new file with the edits applied), however I think this might be completely unused code from a previous editor experiment, so wonder if it's worth it?
(cc @pquit...@google.com @hel...@google.com in case my search wasn't complete and you think `flutter.setWidgetPropertyValue` might be used by IntelliJ)
Thanks for looping me in. I don't see any uses in the plugins either.
It shows up in a list that Alex provided in https://github.com/dart-lang/sdk/issues/60043#issuecomment-3883948834. I think this is a list of unused protocols. What I wasn't sure of at the time is whether these are protocols that aren't used today, or whether these are protocols that have never been used. That is, I don't know whether there might be older versions of the IntelliJ plugin that are making use of them.
Related details
- Danny Tuppeny
- Helin Shiah
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
@FailingTest(
reason:
'This test relies on the formatter failing to return the error, but '
'ChangeBuilder now guards against formatter errors. If this code is '
'in use, we should change the handler to parse the resulting content '
'and check whether there are parse errors, instead of using the '
'formatter for this',
)Phil QuitslundThis relates to my comment at https://github.com/dart-lang/sdk/issues/62866#issuecomment-4054701940
I think we could fix this (by having this handler check for parse errors in a copy of the new file with the edits applied), however I think this might be completely unused code from a previous editor experiment, so wonder if it's worth it?
(cc @pquit...@google.com @hel...@google.com in case my search wasn't complete and you think `flutter.setWidgetPropertyValue` might be used by IntelliJ)
Brian WilkersonThanks for looping me in. I don't see any uses in the plugins either.
It shows up in a list that Alex provided in https://github.com/dart-lang/sdk/issues/60043#issuecomment-3883948834. I think this is a list of unused protocols. What I wasn't sure of at the time is whether these are protocols that aren't used today, or whether these are protocols that have never been used. That is, I don't know whether there might be older versions of the IntelliJ plugin that are making use of them.
It looks like there was code, the last few commits it shows up in seem to be removing it as dead code:
https://github.com/flutter/flutter-intellij/pull/8065
https://github.com/flutter/flutter-intellij/pull/7867
And this Hot UI implementation PR apparently contains it (I can't see it in GH, but that commit shows up if I run `git log -S "setWidgetPropertyValue" --all --full-history`).
https://github.com/flutter/flutter-intellij/pull/4160
So the code does exist in some old IntelliJ plugins (I assume as part of the Hot UI stuff), but I don't know if that was just an experiment or if it shipped without a flag (and if it did, whether those plugins are still supported and/or still work with modern SDKs).
One option is to leave the code, but just allow this test to fail (the result is that if the client tries to edit invalid code in, it will be inserted, instead of being rejected as part of formatting, which doesn't seem like a terrible issue for a defunct feature).
Related details
- Brian Wilkerson
- Helin Shiah
- Phil Quitslund
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
1 comment
I agree, we can just leave the test failing for now. It will require that the user is using an old version of the plugin with a new version of the SDK and types in invalid code. That's probably not going to happen often enough to be concerned about.
Related details
- Danny Tuppeny
- Helin Shiah
- Phil Quitslund
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Danny Tuppeny
- Helin Shiah
- Phil Quitslund
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
I rebased and pushed a fix for the failing test.
I'm not sure if the internal failure was the same cause though, so if that one fails again with this change, someone will have to check the reason.
Thanks!
Related details
- Brian Wilkerson
- Helin Shiah
- Phil Quitslund
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Brian Wilkerson
- Danny Tuppeny
- Helin Shiah
- Phil Quitslund
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted![]( "Open in Gerrit")
- Danny Tuppeny
- Helin Shiah
- Phil Quitslund
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[analysis_server] Don't let format errors fail change building
Some refactors try to format their code, but formatting can fail if there is a parse error anywhere in the file (including in completely unrelated code). This change catches formatting errors and ignores them, so the refactors will continue to work in that case (albeit without formatting).
Fixes https://github.com/dart-lang/sdk/issues/62866
- M pkg/analysis_server/lib/src/lsp/source_edits.dart
- M pkg/analysis_server/test/src/services/flutter/widget_descriptions_test.dart
- M pkg/analysis_server/test/src/services/refactoring/move_selected_formal_parameters_left_test.dart
- M pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
- M pkg/analyzer_plugin/lib/src/utilities/formatter.dart
- M pkg/analyzer_plugin/test/src/utilities/change_builder/change_builder_dart_test.dart
Code-Review: +1 by Brian Wilkerson, +1 by Samuel Rawlins
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |