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

Enable stricter linter rules. #1238

Merged
merged 6 commits into from
May 15, 2022
Merged

Conversation

pablojimpas
Copy link
Contributor

@pablojimpas pablojimpas commented May 15, 2022

Mostly trivial code changes made by running dart fix --apply

The new most important rules enabled are:

All three rules take advantage of relatively recent improvements in the Dart SDK and thus "modernize" the codebase (eg. getting rid of all those var), they can potentially have a positive impact on performance as described in the reference links while also ensuring correctness long term.

I've also re-enabled library_private_types_in_public_api that is present in flutter_lints and was overridden locally.

EDIT:

  • I've now promoted missing_required_param and missing_return to errors in the analyzer and also enabled the "strong-mode" to avoid weak implicit casts and implicit dynamic.
  • A more cosmetic choice of preferring integer literals and using named constants to avoid copying.

Mostly trivial code changes made by running `dart fix --apply`

The rules enabled are:
  - `prefer_final_locals`: https://dart-lang.github.io/linter/lints/prefer_final_locals.html
  - `prefer_final_in_for_each`: https://dart-lang.github.io/linter/lints/prefer_final_in_for_each.html
  - `avoid_dynamic_calls`: https://dart-lang.github.io/linter/lints/avoid_dynamic_calls.html

All three rules take advantage of relatively recent improvements in the
Dart SDK and thus "modernize" the codebase, they can potentially have a
positive impact on performance as described in the reference links.
@pablojimpas
Copy link
Contributor Author

I've also enabled always_use_package_imports since there were only a couple of occurrences of relative imports, this way we enforce consistency on one style going forward.

While I was looking at the imports I've also notice that lib/flutter_map.dart was importing itself (because exports a lot of things) instead of the individual dependencies, that caused a bunch of warnings on pub.dev that lowered the package score:

image

Copy link
Collaborator

@ibrierley ibrierley left a comment

Choose a reason for hiding this comment

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

Looks fine to me, and happy for it to be merged if ok with it. Thanks for the work.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Mostly looks good. A couple of things to maybe change though, suggested below. Happy to approve or change suggestions if you disagree! Many thanks, it's already looking much better.

lib/src/geo/crs/crs.dart Show resolved Hide resolved
lib/src/gestures/gestures.dart Outdated Show resolved Hide resolved
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

In that case, I'm happy to merge as well. Many thanks for your contributions!

@JaffaKetchup JaffaKetchup merged commit f6e2b23 into fleaflet:master May 15, 2022
@pablojimpas pablojimpas deleted the linter-rules branch May 15, 2022 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants