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

feat: Improve prefer_sliver_prefix to prefer_to_include_sliver_in_name #58

Merged
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
6 changes: 3 additions & 3 deletions packages/altive_lints/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ There are also Altive-made rules by custom_lint.
- [avoid\_single\_child](#avoid_single_child)
- [prefer\_clock\_now](#prefer_clock_now)
- [prefer\_dedicated\_media\_query\_methods](#prefer_dedicated_media_query_methods)
- [prefer\_sliver\_prefix](#prefer_sliver_prefix)
- [prefer\_to\_include\_sliver\_in\_name](#prefer_to_include_sliver_in_name)
- [prefer\_space\_between\_elements](#prefer_space_between_elements)
- [Lint rules adopted by altive\_lints and why](#lint-rules-adopted-by-altive_lints-and-why)
- [public\_member\_api\_docs](#public_member_api_docs)
Expand Down Expand Up @@ -252,9 +252,9 @@ var size = MediaQuery.sizeOf(context);
var padding = MediaQuery.viewInsetsOf(context);
```

### prefer_sliver_prefix
### prefer_to_include_sliver_in_name

Prefer to prefix the class name of a Widget that returns a Sliver type Widget with “Sliver”.
Prefer to include ‘Sliver’ in the class name or named constructor of a widget that returns a Sliver-type widget.

This makes it easy for the user to know at a glance that it is a Sliver type Widget, and improves readability and consistency.

Expand Down
4 changes: 2 additions & 2 deletions packages/altive_lints/lib/altive_lints.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import 'src/lints/avoid_shrink_wrap_in_list_view.dart';
import 'src/lints/avoid_single_child.dart';
import 'src/lints/prefer_clock_now.dart';
import 'src/lints/prefer_dedicated_media_query_methods.dart';
import 'src/lints/prefer_sliver_prefix.dart';
import 'src/lints/prefer_space_between_elements.dart';
import 'src/lints/prefer_to_include_sliver_in_name.dart';

/// Returns the Altive Plugin instance.
PluginBase createPlugin() => _AltivePlugin();
Expand All @@ -24,6 +24,6 @@ class _AltivePlugin extends PluginBase {
const PreferClockNow(),
const PreferDedicatedMediaQueryMethods(),
const PreferSpaceBetweenElements(),
const PreferSliverPrefix(),
const PreferToIncludeSliverInName(),
];
}
76 changes: 0 additions & 76 deletions packages/altive_lints/lib/src/lints/prefer_sliver_prefix.dart

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/error/listener.dart';
import 'package:collection/collection.dart';
import 'package:custom_lint_builder/custom_lint_builder.dart';

/// A `prefer_to_include_sliver_in_name` rule that ensures widgets returning
/// a Sliver-type widget include "Sliver" in their class names.
///
/// This naming convention improves code readability and consistency
/// by clearly indicating the widget's functionality
/// and return type through its name.
///
/// The rule also applies if "Sliver" is present in the named constructor,
/// allowing flexibility in how the convention is followed.
///
/// ### Example
///
/// #### BAD:
///
/// ```dart
/// class MyCustomList extends StatelessWidget {
/// @override
/// Widget build(BuildContext context) {
/// return SliverList(...); // LINT
/// }
/// }
/// ```
///
/// #### GOOD:
///
/// ```dart
/// class SliverMyCustomList extends StatelessWidget {
/// @override
/// Widget build(BuildContext context) {
/// return SliverList(...);
/// }
/// }
/// ```
class PreferToIncludeSliverInName extends DartLintRule {
/// Creates a new instance of [PreferToIncludeSliverInName].
const PreferToIncludeSliverInName() : super(code: _code);

static const _code = LintCode(
name: 'prefer_to_include_sliver_in_name',
problemMessage: 'Widgets returning Sliver should include "Sliver" '
'in the class name or named constructor.',
correctionMessage:
'Consider adding "Sliver" to the class name or a named constructor.',
);

@override
void run(
CustomLintResolver resolver,
ErrorReporter reporter,
CustomLintContext context,
) {
context.registry.addClassDeclaration((node) {
final methodBody = node.members
.whereType<MethodDeclaration>()
.firstWhereOrNull((method) => method.name.lexeme == 'build')
?.body;

if (methodBody is! BlockFunctionBody) {
return;
}

final returnStatements =
methodBody.block.statements.whereType<ReturnStatement>();
final returnsSliverWidget = returnStatements.any(
(returnStatement) {
final returnType = returnStatement.expression?.staticType;
final typeName = returnType?.getDisplayString();
return typeName?.startsWith('Sliver') ?? false;
},
);

final className = node.name.lexeme;
final constructorNames = node.members
.whereType<ConstructorDeclaration>()
.map((constructor) => constructor.name?.lexeme)
.whereType<String>();
final hasSliverInClassOrConstructor = className.contains('Sliver') ||
constructorNames.any(
(constructorName) =>
constructorName.toLowerCase().contains('sliver'),
);

if (returnsSliverWidget && !hasSliverInClassOrConstructor) {
reporter.atNode(node, _code);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

badge

The evaluation of returnSliverWidget could be done earlier, allowing for an early return if necessary!

Similarly, the checks for className and constructorNames could also be implemented as early returns, potentially reducing unnecessary member access.

Suggested change
final className = node.name.lexeme;
final constructorNames = node.members
.whereType<ConstructorDeclaration>()
.map((constructor) => constructor.name?.lexeme)
.whereType<String>();
final hasSliverInClassOrConstructor = className.contains('Sliver') ||
constructorNames.any(
(constructorName) =>
constructorName.toLowerCase().contains('sliver'),
);
if (returnsSliverWidget && !hasSliverInClassOrConstructor) {
reporter.atNode(node, _code);
}
if (!returnsSliverWidget) {
return;
}
final className = node.name.lexeme;
if (className.contains('Sliver')) {
return;
}
final constructorNames = node.members
.whereType<ConstructorDeclaration>()
.map((constructor) => constructor.name?.lexeme)
.nonNulls;
final hasSliverConstructor = constructorNames.any(
(name) => name.toLowerCase().contains('sliver'),
);
if (hasSliverConstructor) {
return;
}
reporter.atNode(node, _code);

});
}
}
16 changes: 0 additions & 16 deletions packages/altive_lints/lint_test/lints/prefer_sliver_prefix.dart

This file was deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

badge
I'd like you to write a test for the positive case as well, demonstrating that no warning is issued when the class name contains 'Sliver' and the named constructor includes 'sliver'. This would show the correct behavior in a valid scenario.

Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import 'package:flutter/material.dart';

// expect_lint: prefer_to_include_sliver_in_name
class MyWidget extends StatelessWidget {
const MyWidget({super.key});

@override
Widget build(BuildContext context) {
return SliverList.builder(
itemCount: 10,
itemBuilder: (context, index) {
return const Placeholder();
},
);
}
}

// expect_lint: prefer_to_include_sliver_in_name
class MyWidget2 extends StatelessWidget {
const MyWidget2({super.key, this.maxCount = 10});

const MyWidget2.small({super.key, this.maxCount = 5});

final int maxCount;

@override
Widget build(BuildContext context) {
return SliverList.builder(
itemCount: maxCount,
itemBuilder: (context, index) {
return const Placeholder();
},
);
}
}