New bloc-related lints #458
Conversation
github-actions
bot
added
the
p: leancode_lint
| }); | ||
| } | ||
|
|
||
| void addBloc(void Function(ClassDeclaration node, _BlocData data) listener) { |
There was a problem hiding this comment.
We could add some documentation here. AddBloc is, if I am not wrong, limited to one file. So _BlocData data wouldn't be correct fully. So there is an assumption that the whole bloc with events, states, is in one file. It would be worth mentioning this.
| data.eventElement, | ||
| data.presentationEventElement, | ||
| ]) | ||
| if (element != null) ...{element, ...element.subclasses}, |
There was a problem hiding this comment.
| if (element != null) ...{element, ...element.subclasses}, | |
| ...{?element, ...?element.subclasses}, |
We would have to bump min version of SDK to 3.8 :)
There was a problem hiding this comment.
It has already been bumped 😉
| import 'package:custom_lint_builder/custom_lint_builder.dart'; | ||
| import 'package:leancode_lint/helpers.dart'; | ||
|
|
||
| class BlocConstConstructors extends DartLintRule { |
There was a problem hiding this comment.
What do you think about adding DartFix for this lint? Lint that will create a const constructor.
It could be problematic if Bloc has variables, but I think we have to only handle case where there are no parameters in Bloc's body. And in this case, we only have to create an empty const constructor
|
|
||
| sealed class Test4Event {} | ||
|
|
||
| final class Test4EventFoo extends Test4Event {} |
There was a problem hiding this comment.
Could you add a test case where the state has only one basic class, without any extended classes?
Bloc with only one possible state.
packages/leancode_lint/test/lints_test_app/lib/bloc_const_constructors_test.dart
Show resolved
Hide resolved
|
|
||
| ### `bloc_const_constructors` | ||
|
|
||
| **DO** define unnamed const constructors for bloc state, event, and presentation event classes. |
There was a problem hiding this comment.
Ditto, it would be nice to highlight that this lint works only if bloc is in the same file as states, events etc
|
|
||
| ### `bloc_related_classes_equatable` | ||
|
|
||
| **DO** mix in `EquatableMixin` in bloc state, event, and presentation event classes. |
There was a problem hiding this comment.
It is obvious why it is better to use this mixin, but we could add some info about it here.
There was a problem hiding this comment.
Reasoning from the issue can be mentioned or quoted: felangel/equatable#160
packages/leancode_lint/test/lints_test_app/lib/bloc_related_classes_equatable_test.dart
Show resolved
Hide resolved
packages/leancode_lint/test/lints_test_app/lib/bloc_related_classes_equatable_test.dart
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| class AddMixin extends DartFix { |
There was a problem hiding this comment.
| class AddMixin extends DartFix { | |
| class _AddMixin extends DartFix { |
| analysisError.offset + analysisError.length, | ||
| ' with EquatableMixin', |
There was a problem hiding this comment.
I think this fix is not accurate, finite.
eg.
class A extends B {} // will add EquatableMixin before extends
Additionally, it has to be before implements.
|
|
||
| ```dart | ||
| class MyState { | ||
| MyState(this.value); |
There was a problem hiding this comment.
| MyState(this.value); | |
| const MyState(this.value); |
| class AnotherPackageBloc extends Bloc<int, int> | ||
| with BlocPresentationMixin<int, int> { | ||
| AnotherPackageBloc() : super(0); | ||
| } |
There was a problem hiding this comment.
Please add a case where we have more than one StateClass eg:
class Correct1Cubit extends Cubit<Correct1State>
with BlocPresentationMixin<Correct1State, Correct1Event> {
Correct1Cubit() : super(Correct1State());
}
selaed class Correct1State {}
final class Correct1Initial {}
final class Correct1Loading {}
final class Correct1Loaded {}
There was a problem hiding this comment.
Additionally I think:
final class Correct1Initial {}
final class Correct1Loading {}
final class Correct1Loaded {}
These cases should be legal.
We shouldn't be forced to add in child classes a prefix with the state at the end.
There was a problem hiding this comment.
This lint closely follows our current standards, which specify that the state of <Something>Cubit should be called <Something>State, with subclasses like <Something>State<Variant>
There was a problem hiding this comment.
Ok makes sense that it follows leancode standards but It could be misleading for external people because in bloc documentation examples uses this format:
selaed class ExampleState {}
final class ExampleInitial {}
final class ExampleLoading {}
final class ExampleLoaded {}
Maybe we could add some configuration to allow this?
| return null; | ||
| } | ||
|
|
||
| final stateElement = stateType.element3; |
There was a problem hiding this comment.
What happened here.
Why `Element2? get element3` 😆
| final library1 = element1.library2?.uri; | ||
| final library2 = element2.library2?.uri; | ||
|
|
||
| return library1 != null && library2 != null && library1 == library2; |
There was a problem hiding this comment.
| return library1 != null && library2 != null && library1 == library2; | |
| return library1 != null && library1 == library2; |
Checking if library2 != null is redundant
| bool _isCubitClass(ClassDeclaration node) => | ||
| switch (node.declaredFragment?.element.thisType) { | ||
| final type? => TypeCheckers.cubit.isSuperTypeOf(type), | ||
| _ => false, | ||
| }; |
There was a problem hiding this comment.
This could be extracted to helpers
There was a problem hiding this comment.
We should rename the file to prefer_equatable_mixin.dart
| import 'package:custom_lint_builder/custom_lint_builder.dart'; | ||
| import 'package:leancode_lint/common_type_checkers.dart'; | ||
|
|
||
| class UseEquatableMixin extends DartLintRule { |
There was a problem hiding this comment.
Rename class to match lint name
|
|
||
| ### `prefer_equatable_mixin` | ||
|
|
||
| **DO** mix in `EquatableMixin` instead of extending `Equatable`. |
There was a problem hiding this comment.
So disappointing that a lint is needed for that. Equatable should have been binned ages ago
|
|
||
| ### `bloc_related_classes_equatable` | ||
|
|
||
| **DO** mix in `EquatableMixin` in bloc state, event, and presentation event classes. |
There was a problem hiding this comment.
Reasoning from the issue can be mentioned or quoted: felangel/equatable#160
| if (eventElement is! InterfaceElement2?) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
There doesn't seem to be a dart lint for unrelated type checks when using is, but there is one for pattern matching. As stupid as this looks,
| if (eventElement is! InterfaceElement2?) { | |
| return null; | |
| } | |
| if (stateElement case InterfaceElement2()) { | |
| } else { | |
| return null; | |
| } |
at least it will report a warning if InterfaceElement2 is an unrelated type of stateElement.
This PR introduces six new lints enforcing our new architecture guidelines.
(Also, as a cleanup, all created single-type checkers are moved to one location to avoid duplication.)