enable lint avoid_private_typedef_functions #16356
Conversation
|
I don't understand the value of this one. What's wrong with private typedefs, so long as they don't leak into the public API? |
|
Lint requested by dart-lang/linter#884 . |
| @@ -295,5 +299,3 @@ T _interpolate<T>(List<T> values, double progress, _Interpolator<T> interpolator | |||
| final double t = targetIdx - lowIdx; | |||
| return interpolator(values[lowIdx], values[highIdx], t); | |||
| } | |||
|
|
|||
| typedef T _Interpolator<T>(T a, T b, double progress); | |||
There was a problem hiding this comment.
This is a great example of why I don't think this is a helpful lint. "Interpolator" tells me what this is. T Function(T a, T b, double progress) just looks arbitrary.
It happens to be private here, but we could easily (well, @amirh would surely stop us, but aside from that :-P) make it public, at which point we'd add lots of documentation to this typedef.
There was a problem hiding this comment.
The lint only occurs on private typedefs that are only used once.
Here I agree you loose that this typedef is an Interpolator but IMHO this sort of documentation is still there in the signature of _interpolate with the name of the parameter (interpolator).
T _interpolate<T>(
List<T> values,
double progress,
T Function(T a, T b, double progress) interpolator,
) {There was a problem hiding this comment.
Yeah but in that version it's buried in syntax and hard to spot.
| @@ -480,7 +478,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl | |||
| double _getIntrinsicSize({ | |||
| Axis sizingDirection, | |||
| double extent, // the extent in the direction that isn't the sizing direction | |||
| _ChildSizingFunction childSize // a method to find the size in the sizing direction | |||
| double Function(RenderBox child, double extent) childSize, // a method to find the size in the sizing direction | |||
There was a problem hiding this comment.
similarly here. the very name of the typedef is documentation. I don't see the value of inlining the type.
There was a problem hiding this comment.
One advantage is that you don't have to jump in the source to see what the real signature is.
Perhaps renaming childSize to getChildSize?
| class _ImageProviderResolver { | ||
| _ImageProviderResolver({ | ||
| @required this.state, | ||
| @required this.listener, | ||
| }); | ||
|
|
||
| final _FadeInImageState state; | ||
| final _ImageProviderResolverListener listener; | ||
| final void Function() listener; |
|
Yeah, looking at the patch here I don't think this improves the code. |
There was a problem hiding this comment.
The lint only occurs on private typedefs that are only used once. One advantage is that you don't have to jump in the source to see what the real signature is.
Most of the time the name of the typedef is really close to the name of the parameter. A good name parameter (and dart-doc) can do the job.
I'm ambivalent about typedefs with dart-doc but one can argument that the doc can be moved to the only place the typedef is used.
| @@ -284,7 +284,11 @@ class _PathClose extends _PathCommand { | |||
| // not be smooth enough we can try applying spline instead. | |||
| // | |||
| // [progress] is expected to be between 0.0 and 1.0. | |||
There was a problem hiding this comment.
Side question: is it intentional to have simple comments instead of doc-comments?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the link. I need to read it again since it seems to have received several changed since I read it.
| @@ -295,5 +299,3 @@ T _interpolate<T>(List<T> values, double progress, _Interpolator<T> interpolator | |||
| final double t = targetIdx - lowIdx; | |||
| return interpolator(values[lowIdx], values[highIdx], t); | |||
| } | |||
|
|
|||
| typedef T _Interpolator<T>(T a, T b, double progress); | |||
There was a problem hiding this comment.
The lint only occurs on private typedefs that are only used once.
Here I agree you loose that this typedef is an Interpolator but IMHO this sort of documentation is still there in the signature of _interpolate with the name of the parameter (interpolator).
T _interpolate<T>(
List<T> values,
double progress,
T Function(T a, T b, double progress) interpolator,
) {| @@ -480,7 +478,7 @@ class RenderFlex extends RenderBox with ContainerRenderObjectMixin<RenderBox, Fl | |||
| double _getIntrinsicSize({ | |||
| Axis sizingDirection, | |||
| double extent, // the extent in the direction that isn't the sizing direction | |||
| _ChildSizingFunction childSize // a method to find the size in the sizing direction | |||
| double Function(RenderBox child, double extent) childSize, // a method to find the size in the sizing direction | |||
There was a problem hiding this comment.
One advantage is that you don't have to jump in the source to see what the real signature is.
Perhaps renaming childSize to getChildSize?
|
Closing this PR. I'll do another one to explain why we keep this lint commented in analysis_options.yaml |
No description provided.