-
Notifications
You must be signed in to change notification settings - Fork 24
Issue 190. Implement use nearest context rule #198
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
base: master
Are you sure you want to change the base?
Issue 190. Implement use nearest context rule #198
Conversation
|
Closes #190 |
lib/src/lints/use_nearest_context/models/use_nearest_context_parameters.dart
Outdated
Show resolved
Hide resolved
yurii-prykhodko-solid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We don't seem to handle the cases where the nearest context is a class method:
class Test extends StatefulWidget {
const Test({super.key});
@override
State<Test> createState() => _TestState();
}
class _TestState extends State<Test> {
@override
Widget build(BuildContext context) {
showModalBottomSheet(
context: context,
builder: (BuildContext _) {
return SizedBox.fromSize(size: context.size); // should be linted
},
);
return const Placeholder();
}
}- We should think about what to do with context from global keys. It's usually not a great idea to retrieve external context, so maybe worth to do this:
final key = GlobalKey<State>();
class Test extends StatelessWidget {
const Test({super.key});
@override
Widget build(BuildContext context) {
final outerContext = key.currentContext!;
showModalBottomSheet(
context: outerContext, // LINT
builder: (BuildContext _) {
return SizedBox.fromSize(size: outerContext.size); // LINT
},
);
return const Placeholder();
}
}
|
/gimini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new lint rule, use_nearest_context, which is a valuable addition for Flutter development. The rule correctly identifies situations where a BuildContext from an outer scope is used instead of one available in a nearer, often more appropriate, scope (like a builder callback). The accompanying quick fix to rename the nearest BuildContext parameter is also well-implemented.
The overall implementation is solid, with clear logic in both the rule and the fix, and comprehensive test cases. I have one suggestion for improving the documentation clarity of a data class used by the fix.
Summary of Findings
- Documentation Clarity for
StatementInfo: The doc comments for theStatementInfoclass fields (nameandparameter) could be more specific to clearly convey their roles in the context of the quick fix. This would improve maintainability. - Minor Typo in Comment: In
lib/src/lints/use_nearest_context/fixes/use_nearest_context_fix.dart, line 63, the comment forparameterhas a typo: "parament" should be "parameter". This was not commented on directly due to review settings (low severity). - Quick Fix Message Precision: In
lib/src/lints/use_nearest_context/fixes/use_nearest_context_fix.dart, line 7, the_replaceComment("Rename the BuildContext variable") could be slightly more precise, e.g., "Rename nearest BuildContext parameter". This is a minor stylistic point and was not commented on directly due to review settings (low severity).
Merge Readiness
The pull request is well-implemented and introduces a useful lint rule. There's one medium-severity suggestion regarding documentation clarity for the StatementInfo class. Addressing this would improve the long-term maintainability of the code. Once this minor documentation update is considered, the PR should be in excellent shape for merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by a team member.
| /// Variable name | ||
| final String name; | ||
|
|
||
| /// BuildContext parament | ||
| final SimpleFormalParameter parameter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation for the StatementInfo class could be more descriptive to enhance clarity for future maintainers.
Specifically:
- For the
namefield, "Variable name" is a bit vague. It represents the identifier of theBuildContextthat was used (from an outer scope), and it's the target name for the renaming operation. - For the
parameterfield, "BuildContext parament" contains a typo and could better describe that this is the AST node of the nearestBuildContextparameter that will be renamed.
Could we update these comments for better precision?
/// The identifier name of the BuildContext that was used from an outer scope.
/// The nearest BuildContext parameter will be renamed to this name.
final String name;
/// The [SimpleFormalParameter] node of the BuildContext in the nearest scope.
/// This is the parameter that will be renamed.
final SimpleFormalParameter parameter;
No description provided.