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

[v2.2.0] Stricten Lints & Fix Tests Exceptions #1319

Merged
merged 10 commits into from
Aug 2, 2022
Merged

[v2.2.0] Stricten Lints & Fix Tests Exceptions #1319

merged 10 commits into from
Aug 2, 2022

Conversation

lsaudon
Copy link
Contributor

@lsaudon lsaudon commented Jul 25, 2022

strong-mode is deprecated. It should be replaced by language. Enabling stricter type checks

The exception type 'Null' is not a subtype of type 'HttpHeaders' does not appear in tests anymore.

Add sames rules for example

Replace dynamic by the right type when possible.

@lsaudon lsaudon changed the title linter & fix: strong-mode is obsolete and exception on flutter_map_test chore, fix and refacto: strong-mode is depreacted, exception on flutter_map_test, sames rules for example, replace dynamic Jul 25, 2022
@JaffaKetchup JaffaKetchup changed the title chore, fix and refacto: strong-mode is depreacted, exception on flutter_map_test, sames rules for example, replace dynamic Fix Test Bugs & Tighten Lints Jul 26, 2022
@JaffaKetchup JaffaKetchup changed the title Fix Test Bugs & Tighten Lints Stricten Lints & Fix Tests Exceptions Jul 26, 2022
@JaffaKetchup JaffaKetchup changed the title Stricten Lints & Fix Tests Exceptions [v2.2.0] Stricten Lints & Fix Tests Exceptions Jul 26, 2022
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.

Hi there, and thanks for your contribution. However, there's a few things that need to be changed.

example/lib/pages/overlay_image.dart Outdated Show resolved Hide resolved
@pablojimpas
Copy link
Contributor

The strict-casts mode was introduced in Dart 2.16, right now we support all the way back to 2.12.0

sdk: ">=2.12.0 <3.0.0"

I'm okay with applying the code changes to satisfy new linter rules, but we should either drop support for earlier versions of Dart (which I think it's not worth it) or left the pubspec.yaml untouched.

lib/src/geo/crs/crs.dart Outdated Show resolved Hide resolved
lib/src/geo/crs/crs.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.

I agree with @pablojimpas, I think backwards-compatibility is important, and we need to support Dart 2.12 for now at least.

Also, please stop ignoring close_sinks for the whole library, and just ignore it where I previously asked. It's a useful lint to have, and in this case, it's a false positive.

@lsaudon
Copy link
Contributor Author

lsaudon commented Jul 30, 2022

I agree with @pablojimpas, I think backwards-compatibility is important, and we need to support Dart 2.12 for now at least.

Also, please stop ignoring close_sinks for the whole library, and just ignore it where I previously asked. It's a useful lint to have, and in this case, it's a false positive.

I did not add ignoring close_sinks for the lib. The rule was already there.
I just put the same rules in the example.

Edit:
I don't really understand because according to the history of the analysis_options.yaml file, the rule was never present.

@JaffaKetchup JaffaKetchup self-requested a review July 30, 2022 20:17
analysis_options.yaml 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.

As above.

@JaffaKetchup JaffaKetchup self-requested a review July 30, 2022 20:26
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.

Yeah the checks are still failing, so the 'ignore' line in the tile provider needs to be added back. Let's keep the analysis_options.yaml without the ignore statement though.

@lsaudon
Copy link
Contributor Author

lsaudon commented Jul 30, 2022

Yeah the checks are still failing, so the 'ignore' line in the tile provider needs to be added back. Let's keep the analysis_options.yaml without the ignore statement though.

It's weird because I ran the same commands as in the CLI and this error doesn't appear.
image

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Jul 30, 2022

Yeah - I had this same situation as well. It's because the new package score analyser workflow I added uses a different lint set, so only that one fails: https://github.com/axel-op/dart-package-analyzer. Theoretically, this means that the pub.dev score for this package, should this have been pushed without this change, would have only been 120/130.

@JaffaKetchup JaffaKetchup self-requested a review July 30, 2022 20:38
@JaffaKetchup
Copy link
Member

@pablojimpas Thanks for the heads-up, but I'm a bit confused. How should we support Dart 2.12? When you said 'leave the pubspec untouched', this PR hasn't changed it. Please can you clarify?

Once Dart 2.12 is supported, this should be good to go.

@lsaudon
Copy link
Contributor Author

lsaudon commented Jul 30, 2022

Yeah - I had this same situation as well. It's because the new package score analyser workflow I added uses a different lint set, so only that one fails: https://github.com/axel-op/dart-package-analyzer. Theoretically, this means that the pub.dev score for this package, should this have been pushed without this change, would have only been 120/130.

The package "dart-package-analyzer" does not use the same rules as the package "Pana" that gives the score pub.dev.

I think we should at least have the same rules as the package "dart-package-analyzer".

@JaffaKetchup
Copy link
Member

You might be right, in which case, we should probably just disable this check I added, because I'm not sure you can change the lint rules it uses.
I'll revert my PR or open a new one tomorrow.

@lsaudon
Copy link
Contributor Author

lsaudon commented Jul 30, 2022

You might be right, in which case, we should probably just disable this check I added, because I'm not sure you can change the lint rules it uses.
I'll revert my PR or open a new one tomorrow.

No, don't revert your PR, we can just add the rules and regulations if we have to.

@JaffaKetchup
Copy link
Member

I'm not sure how to do that, so feel free to do so here :)

@lsaudon
Copy link
Contributor Author

lsaudon commented Jul 31, 2022

Ok, I found the rules that pana uses, I added the one that was not in flutter_lints.
I removed the rules that were already defined by flutter_lints.
I sorted the rules.

The rules use by pana analysis_options.dart

@JaffaKetchup
Copy link
Member

OK, that seems good to me I think. @ibrierley Any opinions?

@ibrierley
Copy link
Collaborator

No opinions from me!

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.

LGTM. Quite a large changeset, but I think it's all good. Many thanks for your contributions!

@JaffaKetchup JaffaKetchup merged commit 30ca5de into fleaflet:master Aug 2, 2022
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.

4 participants