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

add exclude params support to avoid_returning_widgets rule #162

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/error/listener.dart';
import 'package:collection/collection.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';
import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_exclude.dart';
import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_parameters.dart';
import 'package:solid_lints/src/models/rule_config.dart';
import 'package:solid_lints/src/models/solid_lint_rule.dart';
import 'package:solid_lints/src/utils/types_utils.dart';
Expand Down Expand Up @@ -48,7 +51,8 @@ import 'package:solid_lints/src/utils/types_utils.dart';
/// }
/// }
/// ```
class AvoidReturningWidgetsRule extends SolidLintRule {
class AvoidReturningWidgetsRule
extends SolidLintRule<AvoidReturningWidgetsParameters> {
/// The [LintCode] of this lint rule that represents
/// the error whether we return a widget.
static const lintName = 'avoid_returning_widgets';
Expand All @@ -61,6 +65,7 @@ class AvoidReturningWidgetsRule extends SolidLintRule {
final rule = RuleConfig(
configs: configs,
name: lintName,
paramsParser: AvoidReturningWidgetsParameters.fromJson,
problemMessage: (_) =>
'Returning a widget from a function is considered an anti-pattern. '
'Unless you are overriding an existing method, '
Expand Down Expand Up @@ -92,11 +97,33 @@ class AvoidReturningWidgetsRule extends SolidLintRule {

final isWidgetReturned = hasWidgetType(returnType);

final isIgnored = _hasIgnored(node, returnType);

final isOverriden = node.declaredElement?.hasOverride ?? false;

if (isWidgetReturned && !isOverriden) {
if (isWidgetReturned && !isOverriden && !isIgnored) {
reporter.reportErrorForNode(code, node);
}
});
}

bool _hasIgnored(Declaration node, DartType returnType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool _hasIgnored(Declaration node, DartType returnType) {
bool _shouldIgnore(Declaration node, DartType returnType) {

or

Suggested change
bool _hasIgnored(Declaration node, DartType returnType) {
bool _isIgnored(Declaration node, DartType returnType) {

What do you think about this naming?

final methodName = node.declaredElement?.name;

final excludedItem = config.parameters.exclude
.firstWhereOrNull((e) => e.methodName == methodName);

if (excludedItem
case AvoidReturningWidgetsExclude(
:final String? className,
)) {
if (className == null) {
return true;
} else {
return returnType.hasIgnoredType(ignoredTypes: {className});
}
}

return false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
final excludedItem = config.parameters.exclude
.firstWhereOrNull((e) => e.methodName == methodName);
if (excludedItem
case AvoidReturningWidgetsExclude(
:final String? className,
)) {
if (className == null) {
return true;
} else {
return returnType.hasIgnoredType(ignoredTypes: {className});
}
}
return false;
final excludedItem = config.parameters.exclude
.firstWhereOrNull((e) => e.methodName == methodName);
if (excludedItem == null) return false;
final className = excludedItem.className;
if (className == null) return false;
return returnType.hasIgnoredType(ignoredTypes: {className});

I would refactor this part into something like this to make it a bit more readable. What do you think?

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/// Model class for AvoidReturningWidgetsExclude parameters
class AvoidReturningWidgetsExclude {
/// The name of the method that should be excluded from the lint.
final String methodName;

/// The name of the class that should be excluded from the lint.
final String? className;

/// Constructor for [AvoidReturningWidgetsExclude] model
const AvoidReturningWidgetsExclude({
required this.methodName,
required this.className,
});

///
factory AvoidReturningWidgetsExclude.fromJson(
Map<dynamic, dynamic> json,
) {
return AvoidReturningWidgetsExclude(
methodName: json['method_name'] as String,
className: json['class_name'] as String?,
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import 'package:solid_lints/src/lints/avoid_returning_widgets/models/avoid_returning_widgets_exclude.dart';

/// A data model class that represents the "avoid returning widgets" input
/// parameters.
class AvoidReturningWidgetsParameters {
/// A list of methods that should be excluded from the lint.
final List<AvoidReturningWidgetsExclude> exclude;

/// Constructor for [AvoidReturningWidgetsParameters] model
AvoidReturningWidgetsParameters({
required this.exclude,
});

/// Method for creating from json data
factory AvoidReturningWidgetsParameters.fromJson(Map<String, dynamic> json) {
final exclude = <AvoidReturningWidgetsExclude>[];

for (final item in (json['exclude'] as Iterable?) ?? []) {
if (item is Map && item['method_name'] is String) {
exclude.add(AvoidReturningWidgetsExclude.fromJson(item));
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (final item in (json['exclude'] as Iterable?) ?? []) {
if (item is Map && item['method_name'] is String) {
exclude.add(AvoidReturningWidgetsExclude.fromJson(item));
}
}
final excludeList = json['exclude'] as Iterable? ?? [];
for (final item in excludeList) {
if (item is Map) {
exclude.add(AvoidReturningWidgetsExclude.fromJson(item));
}
}

I don't think we need to check the item structure here since it is not a responsibility of this class.

return AvoidReturningWidgetsParameters(
exclude: exclude,
);
}
}
11 changes: 11 additions & 0 deletions lint_test/avoid_returning_widget_test/analysis_options.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
analyzer:
plugins:
- ../custom_lint

custom_lint:
rules:
- avoid_returning_widgets:
exclude:
- method_name: excludeWidget
class_name: SizedBox
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class_name means class in which we are ignoring returning widgets from the method_name, but not the return type of the ignored method_name.

- method_name: excludeWidget2
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,7 @@ class MyWidget extends BaseWidget {
Widget build() {
return Offstage();
}

SizedBox excludeWidget() => const SizedBox();

Container excludeWidget2() => Container();
Loading