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

migrate from pedantic to flutter_lints #1183

Merged
merged 1 commit into from
Apr 26, 2022
Merged

migrate from pedantic to flutter_lints #1183

merged 1 commit into from
Apr 26, 2022

Conversation

a14n
Copy link
Contributor

@a14n a14n commented Mar 10, 2022

No description provided.

@ibrierley
Copy link
Collaborator

Interesting, this was far off my radar. I don't really know much on this side, but looking at https://stackoverflow.com/questions/67734486/pedantic-vs-flutter-lint-which-package-to-use-and-can-they-also-be-combined it says pedantic is deprecated.
Thanks for the work on this @a14n are there any consequences you can think of, is anything flutter version related for example that could crop up ?

@a14n
Copy link
Contributor Author

a14n commented Mar 11, 2022

The semantics is almost the same and I don't think it will cause real issue. The only place where I change some code is around MapController. I added some methods to allow FlutterMapState to deal with MapController and not directly MapControllerImpl. But I do think there was a real problem about how MapController is handled (but I am not going to address that in this PR to avoid too complicated review)

a14n
a14n commented Mar 11, 2022
View reviewed changes
lib/flutter_map.dart
Comment on lines +140 to +142
StreamSink<MapEvent> get mapEventSink;
set state(MapState state);
void dispose();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are the 3 added methods.

@JaffaKetchup
Copy link
Member

Thanks for your contribution!
To appease the checks, can you run: dart format . in the root? Many thanks again.

a14n
a14n commented Mar 12, 2022
View reviewed changes
example/lib/pages/tile_loading_error_handle.dart
@@ -50,7 +50,7 @@ class _TileLoadingErrorHandleState extends State<TileLoadingErrorHandle> {
tileProvider: const NonCachingNetworkTileProvider(),
errorTileCallback: (Tile tile, error) {
if (_needLoadingError) {
WidgetsBinding.instance.addPostFrameCallback((_) {
WidgetsBinding.instance?.addPostFrameCallback((_) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

on master branch of flutter WidgetsBinding.instance is now not nullable (that's why this error occured)

@pablojimpas
Copy link
Contributor

Definitely a good move to migrate from pedantic as it is deprecated.
However, I will suggest choosing a stricter set of rules like the ones from the lint or very_good_analysis packages.

Here is a detailed comparison of linting packages https://rydmike.com/blog_flutter_linting.html

In the past I've done some tinkering trying to fix all "errors" that appear in this package using very_good_analysis and there are some interesting ones worth fixing, those regarding to avoiding dynamic calls or dynamic typing in general can have impact in performance for example.

@a14n
Copy link
Contributor Author

a14n commented Mar 14, 2022

@pablojimpas I have no problem with enabling more lints but choosing the right set of rules is not the subject of this PR IMHO. For now I rely on the most consensual set for flutter packages (btw as author of several of the dartlang lints there are definitelly some of them I'll never enable on my code). Note that flutter_lints is constantly evolving and that more lints will be enable in the future.

@pablojimpas
Copy link
Contributor

@pablojimpas I have no problem with enabling more lints but choosing the right set of rules is not the subject of this PR IMHO. For now I rely on the most consensual set for flutter packages (btw as author of several of the dartlang lints there are definitelly some of them I'll never enable on my code). Note that flutter_lints is constantly evolving and that more lints will be enable in the future.

Okey I see your point, I am fine with the choice of flutter_lints as it is the most used one. Later on we could evaluate if it is worth to enable some particular extra rule for this project.

Also, I would keep an eye on flutter/packages#1165

@JaffaKetchup
Copy link
Member

Will try to test later today. Note it might take longer than usual, as there are a lot of changes.

@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Apr 21, 2022
@a14n
Copy link
Contributor Author

a14n commented Apr 25, 2022

ping

@JaffaKetchup
Copy link
Member

Sorry for the long delay. Will try to handle this soon :)

ibrierley
ibrierley approved these changes Apr 25, 2022
View reviewed changes
Copy link
Collaborator

@ibrierley ibrierley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Did a bit more testing on this. I like it, and appreciate the work. Happy for anyone else to do a bit more testing as well though first, as there's quite a few files, but most look straightforward.

@JaffaKetchup
Copy link
Member

@ibrierley if you're happy, don't worry about my review.
@a14n thanks for the great contribution, and sorry it took so long to review (I didnt even get round to it in the end!)

@github-actions github-actions bot removed the Stale label Apr 26, 2022
@ibrierley ibrierley merged commit f2b9271 into fleaflet:master Apr 26, 2022
@a14n a14n deleted the flutter_lints branch April 26, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrierley ibrierley ibrierley 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.
4 participants
@a14n @ibrierley @JaffaKetchup @pablojimpas