Skip to content
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

quick fix for diagnostic_describe_all_properties #38633

Closed
9 tasks done
pq opened this issue Sep 28, 2019 · 7 comments
Closed
9 tasks done

quick fix for diagnostic_describe_all_properties #38633

pq opened this issue Sep 28, 2019 · 7 comments
Assignees
Labels
analyzer-linter Issues with the analyzer's support for the linter package analyzer-quick-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. customer-flutter

Comments

@pq
Copy link
Member

pq commented Sep 28, 2019

Follow-up from: flutter/flutter#41513 (comment).

@jacob314: feel free to chime in w/ any implementation notes here. Any heuristics for code generation would be greatly appreciated!


Property subclasses to support

  • BooleanProperty -- bool (23aca57)
  • ColorProperty -- for Color (0a63bd4)
  • EnumProperty -- for any enum class (512d763)
  • IntProperty -- int (512d763)
  • DoubleProperty -- double (512d763)
  • IterableProperty -- any iterable (0a63bd4)
  • StringProperty -- string (512d763)
  • TransformProperty -- Matrix4 (0a63bd4)
  • DiagnosticsProperty for any T that doesn't match one of the other cases (69b612b)
@pq pq added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-linter Issues with the analyzer's support for the linter package analyzer-quick-fix customer-flutter labels Sep 28, 2019
@pq pq self-assigned this Sep 28, 2019
@bwilkerson
Copy link
Member

@goderbauer Could you send us a couple of before/after pairs? That would likely be very helpful for informing what the fix should do.

@jacob314
Copy link
Member

I can provide all the before and after pairs.
https://api.flutter.dev/flutter/foundation/DiagnosticsProperty-class.html
gives a list of all subclasses.
ColorProperty -- for Color
EnumProperty -- for any enum class
IntProperty -- int
DoubleProperty -- double
IterableProperty -- any iterable
StringProperty -- string
TransformProperty -- Matrix4
DiagnosticsProperty for any T that doesn't match one of the other cases.

For each case the quick fix is to either add a line at the end of the existing debugFillProperties method or to override the debugFillProperties method and then add the property.
For example, for an int property you would add the line
properties.add(IntProperty('myPropertyName', myPropertyName));
method or add a new one. The big time savings for the quick fix is otherwise we were finding it was a real pain to for every single property navigate to the debugFillProperties method to add the property. Sometimes it wasn't obvious where the method was and almost all of these migrations are mechanical.

@bwilkerson
Copy link
Member

That should be fairly easy to automate. Given the amount of work there is it sounds like we should also consider enabling that via dartfix, which could apply the fix in bulk.

@jacob314
Copy link
Member

jacob314 commented Sep 29, 2019 via email

@bwilkerson
Copy link
Member

How do you manually determine the default value for a property? Is it possible to statically determine the right default value for a property? Maybe by looking at the field's initializer and/or the constructors?

How do you manually determine which properties to add? I would have assumed that the lint was able to do this in order to not produce false positives. If that's not the case then we should at least be able to add an 'ignore' comment for properties that shouldn't be added. If you do that, then those diagnostics will be suppressed and given that fixes are only applied where a diagnostic is reported, we should still be able to use dartfix after marking the properties that shouldn't be added.

dart-bot pushed a commit that referenced this issue Oct 1, 2019
very constrained support for fixing `diagnostic_describe_all_properties` for the boolean field case.

still todo:

* tests for no debugFillProperties method
* tests for getters
* support for ColorProperty -- for Color
* support for EnumProperty -- for any enum class
* support for IntProperty -- int
* support for DoubleProperty -- double
* support for IterableProperty -- any iterable
* support for StringProperty -- string
* support for TransformProperty -- Matrix4
* support for DiagnosticsProperty for any T that doesn't match one of the other cases

(at least)

See: #38633




Change-Id: Ie6875f49914e54d3b4539fcb51a231da688bdf18
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119536
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
pq referenced this issue Oct 2, 2019
Change-Id: Iac5523b90f3f753107cf5dc22c3cd211a21ae040
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119681
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Oct 3, 2019
Adds support for:

* ints
* doubles
* enums
* Strings

Up next: Colors, Iterables, Transforms and a catch-all.

Also note the TODO to change how types are being written (will be needed for Iterables).

See: #38633

Change-Id: I9d545ce9e090059ae330f9ae5dadf8e7d4b373c7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119725
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Oct 3, 2019
See: #38633

Change-Id: I8d7baa32bcea22f2c162bd9d77c7dd0b95b308de
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119901
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
dart-bot pushed a commit that referenced this issue Oct 4, 2019
…f undefined

See: #38633

Change-Id: If82922a38243e144ad2ec0038d8642c17f458df8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/120140
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Oct 4, 2019
See: #38633

Change-Id: I5fd1328a5ad234e6216631007ecbcc459bfa36e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/120340
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
@pq
Copy link
Member Author

pq commented Oct 8, 2019

Video demonstrating the simple/common case...

diagProps1

🤘

/cc @jacob314

@pq
Copy link
Member Author

pq commented Oct 8, 2019

For reference: dart-lang/linter#1420.

dart-bot pushed a commit that referenced this issue Oct 10, 2019
See: #38633

Change-Id: Ie10dcde92e0e054271fe0e965ed810ac3834120b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/121160
Commit-Queue: Phil Quitslund <pquitslund@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Oct 15, 2019
See: #38633

Follow-up from: flutter/flutter#42447 (comment)


Change-Id: Idd3cbc4b3419e792d6b81e02bd556b35faaef8c7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/121770
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Phil Quitslund <pquitslund@google.com>
@pq pq closed this as completed Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package analyzer-quick-fix area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. customer-flutter
Projects
None yet
Development

No branches or pull requests

3 participants