contribute a fix_data.yaml file to enable 'dart fix' renames #141

devoncarew · 2024-01-12T16:41:28Z

contribute a fix_data.yaml file to enable 'dart fix' renames
document the class renames from dart:html to package:web
related to investigate dart fix renames for this package #139

@keertip - can you review the fix_data.yaml file? I have some questions about it (in-line).
cc @bwilkerson and @jacob314 as we discussed this yesterday

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

devoncarew · 2024-01-12T16:42:14Z

lib/fix_data.yaml

+  - title: "Rename to 'HTMLAnchorElement'"
+    date: 2024-01-12
+    element:
+      uris: [ 'package:web/web.dart' ]


@keertip - should this be package:web/web.dart? web.dart? I see different things in the flutter package dart fix data.

Either ought to be fine. The shorter form is an "abbreviated URI". See https://github.com/flutter/flutter/wiki/Data-driven-Fixes#uri.

Either should be fine. Though we do have this open issue - dart-lang/sdk#52233.

Hmm - is that issue a blocker? Does it mean we should include both types of urls in the fix data file (web.dart and package:web/web.dart)? Is this is going to affect us it would be great to prioritize that fix for the next release.

devoncarew · 2024-01-12T16:42:33Z

example/example.dart

@@ -5,6 +5,11 @@
 import 'package:web/web.dart';

 void main() {
-  final div = document.querySelector('div') as HTMLDivElement;
+  // ignore: unused_local_variable
+  final HtmlDocument foo = document;


This location does offer a quick fix, to rename to Document.

devoncarew · 2024-01-12T16:43:35Z

example/example.dart

+  // ignore: unused_local_variable
+  final HtmlDocument foo = document;
+
+  final doc = document as HtmlDocument;


This location does not offer a quick fix. Should I define the fix data differently? Is the rename fix expected to cover this case?

cc @keertip

I would have expected the rename change to cover this case. It looks like a bug to me.

As Brian said, it should, otherwise it is a bug. Could you open an issue to capture this?

dart-lang/sdk#54617

devoncarew · 2024-01-12T16:49:56Z

Somewhat related - and I can open a separate issue for this - quick fix will currently un-do the work of the dart fix fixes. When I open the quick fixes available at a ctor reference to an older symbol from dart:html (w/ just package:web in the imports, not dart:html):

The 3rd item above is what we want the user to select - to rename the older symbol name to the new (HTMLDivElement) name. However the first item suggested - adding an import for dart:html - will actually work against the migration, and will likely be confusing to casual users. We should probably stop suggesting adding imports for the legacy web libraries (eg, https://api.dart.dev/main/a5758a0dc373e137ee2cf486b5a537fbd5696a95/index.html).

devoncarew · 2024-01-12T16:50:55Z

It would be great to be able to bump the 'rename' quick fix to be first in the list in any case - for it to be suggested ahead of the 'create new class' fix.

bwilkerson

Consider defining tests for each of the transformations as per https://github.com/flutter/flutter/wiki/Data-driven-Fixes#testing. I strongly recommend having at least one .dart file per transform, even though the flutter team has chosen to put the tests for multiple transforms in a single file. I think it makes it much easier to debug problems when the test files are smaller.

bwilkerson · 2024-01-12T17:05:51Z

example/example.dart

+  // ignore: unused_local_variable
+  final HtmlDocument foo = document;
+
+  final doc = document as HtmlDocument;


I would have expected the rename change to cover this case. It looks like a bug to me.

bwilkerson · 2024-01-12T17:08:47Z

lib/fix_data.yaml

+  - title: "Rename to 'HTMLAnchorElement'"
+    date: 2024-01-12
+    element:
+      uris: [ 'package:web/web.dart' ]


Either ought to be fine. The shorter form is an "abbreviated URI". See https://github.com/flutter/flutter/wiki/Data-driven-Fixes#uri.

devoncarew · 2024-01-12T20:40:20Z

Consider defining tests for each of the transformations as per https://github.com/flutter/flutter/wiki/Data-driven-Fixes#testing.

Thanks for the feedback and the idea! I added content in a test_fixes/ dir. It did find 1/2 dozen quick fixes that were not taking effect - places where we had introduced typedefs to help w/ migrations.

bwilkerson · 2024-01-12T21:25:08Z

Glad we found the bugs.

srujzs

Awesome, thanks for working on this! I commented on the other issue on why we might be missing some of the mappings. (I'm approving from my perspective - better to have some of the analyzer folks approve before submitting)

keertip

LGTM for the fix_data.yaml file. Please do test that dart fix works if you change the uri from 'package:'.

devoncarew · 2024-01-14T19:24:56Z

LGTM for the fix_data.yaml file. Please do test that dart fix works if you change the uri from 'package:'.

I did try updating the fix_data references to use web.dart (from package:web/web.dart). The fixes didn't work after that - either from the editor (in the example file) or via the cli testing (--compare-to-golden) technique. Keeping the longer form for now.

@js

Generate extension type bindings Generate extension types instead of classes Refactors some code around overrides so we don't emit @js unless they're needed. Updates code_builder dep to 4.10.0 and dart_style to 2.3.4. Clean up JS supertype generation contribute a fix_data.yaml file to enable 'dart fix' renames (dart-lang#141) contribute a fix_data.yaml file to enable 'dart fix' renames collect MDN API information (dart-lang#130) collect MDN info for use in API documentation Redeprecate deprecated APIs in 0.4.1 (dart-lang#142) Undeprecate some APIs and prepare for publish of 0.4.2 (dart-lang#140) move bindings_generator/ to tool/generator/ (dart-lang#138) rev to 0.4.1 in preparation for publishing (dart-lang#135) Update generate copyright year to 2024 (dart-lang#136) Change-Id: I0f90fc08fd7871f8331f42c158cb7ba34e6bf887

contribute a fix_data.yaml file to enable 'dart fix' renames

b8ecaec

devoncarew marked this pull request as draft January 12, 2024 16:41

devoncarew commented Jan 12, 2024

View reviewed changes

bwilkerson reviewed Jan 12, 2024

View reviewed changes

devoncarew mentioned this pull request Jan 12, 2024

investigate dart fix renames for this package #139

Closed

devoncarew added 2 commits January 12, 2024 12:34

add tests for the dart_fix.yaml file

99f2077

update the changelog

3cc6dfc

devoncarew marked this pull request as ready for review January 12, 2024 20:40

edit readme

5755902

devoncarew requested a review from srujzs January 12, 2024 20:53

devoncarew mentioned this pull request Jan 12, 2024

no longer suggest package imports for dart:html (and related) packages dart-lang/sdk#54610

Closed

srujzs approved these changes Jan 12, 2024

View reviewed changes

devoncarew added 2 commits January 12, 2024 15:26

merge to main

c94fd0b

add several more renames

4c1aab0

devoncarew requested review from keertip and bwilkerson January 13, 2024 00:22

keertip approved these changes Jan 14, 2024

View reviewed changes

devoncarew merged commit d17fdd2 into main Jan 14, 2024

devoncarew deleted the fix_data.yaml branch January 14, 2024 19:25

devoncarew mentioned this pull request Jan 14, 2024

[dart fix] rename change not suggested for casts dart-lang/sdk#54617

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contribute a fix_data.yaml file to enable 'dart fix' renames #141

contribute a fix_data.yaml file to enable 'dart fix' renames #141

devoncarew commented Jan 12, 2024 •

edited

Loading

devoncarew Jan 12, 2024

bwilkerson Jan 12, 2024

keertip Jan 14, 2024

devoncarew Jan 14, 2024

devoncarew Jan 12, 2024

devoncarew Jan 12, 2024

bwilkerson Jan 12, 2024

keertip Jan 14, 2024

devoncarew Jan 14, 2024

devoncarew commented Jan 12, 2024

devoncarew commented Jan 12, 2024

bwilkerson left a comment

bwilkerson Jan 12, 2024

bwilkerson Jan 12, 2024

devoncarew commented Jan 12, 2024

bwilkerson commented Jan 12, 2024

srujzs left a comment •

edited

Loading

keertip left a comment

devoncarew commented Jan 14, 2024

contribute a fix_data.yaml file to enable 'dart fix' renames #141

contribute a fix_data.yaml file to enable 'dart fix' renames #141

Conversation

devoncarew commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew commented Jan 12, 2024

devoncarew commented Jan 12, 2024

bwilkerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devoncarew commented Jan 12, 2024

bwilkerson commented Jan 12, 2024

srujzs left a comment • edited Loading

Choose a reason for hiding this comment

keertip left a comment

Choose a reason for hiding this comment

devoncarew commented Jan 14, 2024

devoncarew commented Jan 12, 2024 •

edited

Loading

srujzs left a comment •

edited

Loading