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.

Sign up for GitHub

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

Jump to bottom

feat(riverpod_lint): added avoid use ref inside dispose #2701

Merged
merged 18 commits into from
Jul 27, 2023
Merged

feat(riverpod_lint): added avoid use ref inside dispose #2701

merged 18 commits into from
Jul 27, 2023

Conversation

LeonardoRosaa
Copy link
Contributor

It's a good practice to avoid using the WidgetRef in the dispose method to avoid some problems although it isn't every usage that creates some problems.

Identifies the dispose method by name and the @override annotation once a time the WidgetRef is only used in the State class.

Reference: #2542

@codecov
Copy link

codecov bot commented Jul 6, 2023 •
edited
Loading

Codecov Report

Merging #2701 (5e5e42c) into master (776df50) will not change coverage.
Report is 16 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2701   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files          53       53           
  Lines        2252     2252           
=======================================
  Hits         2145     2145           
  Misses        107      107

@rrousselGit
Copy link
Owner

Hello!
The code isn't formatted. Could you fix that?

Also we would need tests. See the riverpod_lint_flutter_test folder

@LeonardoRosaa
Copy link
Contributor Author

Hello! The code isn't formatted. Could you fix that?

Also we would need tests. See the riverpod_lint_flutter_test folder

Yes. I would write tests but I got some errors after executing the flutter test command:

00:02 +0: /Users/leonardo/www/riverpod/packages/riverpod_lint_flutter_test/test/ensuire_build_test.dart: ensure_build                                                                
Running: `git rev-parse --show-toplevel` in `/Users/leonardo/www/riverpod/packages/riverpod_lint_flutter_test`
00:02 +1: /Users/leonardo/www/riverpod/packages/riverpod_lint_flutter_test/test/goldens/fixes/provider_dependencies_test.dart: Verify that @riverpod classes extend the generated typedef
[exception: PathNotFoundException(path=/Users/leonardo/flutter/bin/cache/artifacts/engine/lib/_internal/sdk_library_metadata/lib/libraries.dart; message=Cannot open file)][stackTrace: #0      _PhysicalFile.readAsStringSync (package:analyzer/file_system/physical_file_system.dart:159:7)
#1      FolderBasedDartSdk.initialLibraryMap (package:analyzer/src/dart/sdk/sdk.dart:521:41)
#2      new FolderBasedDartSdk (package:analyzer/src/dart/sdk/sdk.dart:420:18)
#3      ContextBuilderImpl._createSdk (package:analyzer/src/dart/analysis/context_builder.dart:192:21)
#4      ContextBuilderImpl.createContext (package:analyzer/src/dart/analysis/context_builder.dart:104:15)
#5      new AnalysisContextCollectionImpl (package:analyzer/src/dart/analysis/analysis_context_collection.dart:95:36)
#6      _createAnalysisContext (package:analyzer/dart/analysis/utilities.dart:127:42)
#7      resolveFile2 (package:analyzer/dart/analysis/utilities.dart:117:7)
#8      main.<anonymous closure> (file:///Users/leonardo/www/riverpod/packages/riverpod_lint_flutter_test/test/goldens/fixes/provider_dependencies_test.dart:22:28)
#9      testGolden.<anonymous closure> (file:///Users/leonardo/www/riverpod/packages/riverpod_lint_flutter_test/test/golden.dart:29:31)
#10     Declarer.test.<anonymous closure>.<anonymous closure> (package:test_api/src/backend/declarer.dart:215:19)
<asynchronous suspension>
#11     StackZoneSpecification._registerUnaryCallback.<anonymous closure> (package:stack_trace/src/stack_zone_specification.dart:124:15)
<asynchronous suspension>
]
[exception: PathNotFoundException(path=/Users/leonardo/flutter/bin/cache/artifacts/engine/lib/_internal/libraries.dart; message=Cannot open file)][stackTrace: #0      _PhysicalFile.readAsStringSync (package:analyzer/file_system/physical_file_system.dart:159:7)
#1      FolderBasedDartSdk.initialLibraryMap (package:analyzer/src/dart/sdk/sdk.dart:521:41)
#2      new FolderBasedDartSdk (package:analyzer/src/dart/sdk/sdk.dart:420:18)
#3      ContextBuilderImpl._createSdk (package:analyzer/src/dart/analysis/context_builder.dart:192:21)
#4      ContextBuilderImpl.createContext (package:analyzer/src/dart/analysis/context_builder.dart:104:15)
#5      new AnalysisContextCollectionImpl (package:analyzer/src/dart/analysis/analysis_context_collection.dart:95:36)
#6      _createAnalysisContext (package:analyzer/dart/analysis/utilities.dart:127:42)
#7      resolveFile2 (package:analyzer/dart/analysis/utilities.dart:117:7)
#8      main.<anonymous closure> (file:///Users/leonardo/www/riverpod/packages/riverpod_lint_flutter_test/test/goldens/fixes/provider_dependencies_test.dart:22:28)
#9      testGolden.<anonymous closure> (file:///Users/leonardo/www/riverpod/packages/riverpod_lint_flutter_test/test/golden.dart:29:31)
#10     Declarer.test.<anonymous closure>.<anonymous closure> (package:test_api/src/backend/declarer.dart:215:19)
<asynchronous suspension>
#11     StackZoneSpecification._registerUnaryCallback.<anonymous closure> (package:stack_trace/src/stack_zone_specification.dart:124:15)
<asynchronous suspension>
]

Flutter version:

Flutter 3.10.5 • channel stable • https://github.com/flutter/flutter.git
Framework • revision 796c8ef792 (3 weeks ago) • 2023-06-13 15:51:02 -0700
Engine • revision 45f6e00911
Tools • Dart 3.0.5 • DevTools 2.23.1

@AhmedLSayed9
Copy link
Contributor

I got the same error on my PR and I'm stuck on this.

@rrousselGit
Copy link
Owner

Not sure how you got that error. The CI is running tests and works just fine.

@LeonardoRosaa
Copy link
Contributor Author

Not sure how you got that error. The CI is running tests and works just fine.

Do you run these tests only in the CI?

@LeonardoRosaa
Copy link
Contributor Author

Hello @rrousselGit,

I am not sure that I wrote the right tests but can you check?

Thanks.

@AhmedLSayed9
Copy link
Contributor

@LeonardoRosaa Did you write the golden test manually?

@LeonardoRosaa
Copy link
Contributor Author

@LeonardoRosaa Did you write the golden test manually?

Yes, I did.

@AhmedLSayed9
Copy link
Contributor

I see.
I think I'd go with that as I can't run tests too. Thanks!

@rrousselGit
Copy link
Owner

Thanks for the test! We'll probably need a few more cases to make sure we have 100% coverage.
I've already suggested one. There may be more needed. I'd suggest looking at the core and see if anything isn't tested

@LeonardoRosaa
Copy link
Contributor Author

Thanks for the test! We'll probably need a few more cases to make sure we have 100% coverage. I've already suggested one. There may be more needed. I'd suggest looking at the core and see if anything isn't tested

I did some improvements. Can you check it?

@rrousselGit
Copy link
Owner

Could you document this lint in the README?

@LeonardoRosaa
Copy link
Contributor Author

Could you document this lint in the README?

I wrote some documentation. But, can you explain in more detail why to avoid using ref in the dispose method?

rrousselGit
rrousselGit reviewed Jul 25, 2023
View reviewed changes
packages/riverpod_lint/lib/src/lints/avoid_use_ref_inside_dispose.dart Outdated

const disposeMethod = 'dispose';

class AvoidUseRefInsideDispose extends RiverpodLintRule {
Copy link
Owner

@rrousselGit rrousselGit Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you rename the class and file name to match the lint code? avoid_ref_inside_state_dispose.dart and AvoidRefInsideStateDispose

rrousselGit
rrousselGit approved these changes Jul 25, 2023
View reviewed changes
Copy link
Owner

@rrousselGit rrousselGit left a comment

Choose a reason for hiding this comment

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

LGTM once the lint class & filename are renamed

Thanks for this :)

@LeonardoRosaa
Copy link
Contributor Author

LGTM once the lint class & filename are renamed

Thanks for this :)

Done :)

@rrousselGit
Copy link
Owner

Epic. Thanks!

@rrousselGit rrousselGit merged commit 4fb4fcc into rrousselGit:master Jul 27, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rrousselGit rrousselGit rrousselGit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LeonardoRosaa @rrousselGit @AhmedLSayed9