Linters setup fix and lint rules review for Flutter 2.5

rydmike commented 3 years ago

Linters and Flutter 2.5

This is a review of linters to prepare for Flutter 2.5. I made a pull request, here (https://github.com/weta-vn/advance_image_picker/pull/23) that contains atomic updates, for your review to the linter setup and linter issues discussed below. This should help you with getting the linter working fully and make the code Flutter 2.5 ready as well.

Atomic commits included in the the pull request:

Photo manager version upgrade (minor) (https://github.com/rydmike/advance_image_picker/commit/8d20e47bf7466b67da8221dae6a26cd0cd152361)
Remove two lint rules that were put on manually that ARE already on (https://github.com/rydmike/advance_image_picker/commit/8d20e47bf7466b67da8221dae6a26cd0cd152361), a typo correction (https://github.com/rydmike/advance_image_picker/commit/c200ac7aaf7b4e985d25a19d6646e2137b07c223) together with Dart SDK 2.14 fixes the issues with the linter not being triggered correctly so those extra true rules are no longer needed, they are ON already. The idea with this linter setup is to only have to turn OFF rules in analysis_options.yaml never on, all available rules are put ON by default in all_lint_rules.yaml.
Added two new rules to all_lint_rules.yaml released by Dart team. (https://github.com/rydmike/advance_image_picker/commit/e5df4359bdd0ea44d865bb916f1ac233ca6d6353)
Set SDK to 2.14.0 (https://github.com/rydmike/advance_image_picker/commit/2308cfdc532fc025f3ef704caf7b17c4df39d091)

The above fixes led to that there were a massive 598 linter hints triggered. They should have been triggered already with past lint setup, but due to the issue mentioned above they were not.

To reduce the lint warnings dart fix --apply was run as well as removing the in Flutter 2.5.0 deprecated AppBar API usage that was still used temporarily via (https://github.com/weta-vn/advance_image_picker/pull/11/commits/17cd5c97d7466abd3d0ad43709fe52c2efaad6a2), but flagged as should be removed in later version, this also removes the deprecated AppBar backwardsCompatibility API usage.

These changes brought the lint errors to "only" 177 remaining.

After this I did a large amount of manual lint fixes as well, they are listed as atomic commits with comments in the pull request. Those fixes were very basic that the IDE could do pretty automatically but dart fix --apply could not.

What is left?

This still leaves 3 warning and 81 lint hints un-resolved. I left them un-fixed on purpose, some are just doc comments for you to simply add (it will improve your doc API percentage in pub.dev ) but others are good to review manually, most of them are straight forward. For the 3 cases of "warning: Don't reassign references to parameters of functions or methods." you may want to add and ignore of the lint for the line where you mutate the parameter value, if that is what you want to do. just put a:

// ignore: parameter_assignments

before the line where you need to do it and the warning goes away and at the same time the ignore is a good way to document that it is intentional. Alternatively rethink the re-assignment of the parameter, make the parameter final and create a local copy of it that you change if so needed.

The rest of the still open fixes are quite simple, but I can certainly advice if you need help.

I did a test build and tested the picker in an app, with all above changes applies. As far as I could see everything still worked as before. 😃

BTW yes the avoid lines longer than 80 chars is very annoying. However, at least not long ago, a package pub.dev score was penalized 10 points if it was not used, since the dartfmt that is used by pub.dev uses 80 chars as max allowed line as the format it check against when giving points for using dartfmt. I have not checked if they have abandoned that part of the dartfmt check yet. I don't think so, but there was some debate about using it raised in issues. Could be good to test if it is still used.

I think that is the format issue I see on pub.dev, might not be related to 80 char length anymore:

Definitely worth testing just turning it OFF and see if it affects the score ore not, there is already in analysis_options.yaml a placeholder, just uncomment it.

Remaining lint warnings and hints to be corrected:

weta-vn commented 3 years ago

@rydmike Thank you very much. We'll confirm & merge asap.

rydmike commented 3 years ago

Did some minor edits to the description above, mostly on the 80 char lint topic.

rydmike commented 3 years ago

I realize this kind of merge might be tricky for your dev branches and therefore not feasible right now. In that case you can use my steps as a guide.

Still 90% of the work for complete lint implmentation and migration is done, plus some Flutter 2.5 deprecation migrations. I just left a few lint warnings in there for you to fix and did not add more API docs, better that you do that. The API doc lint is handy since it tells you were they are missing, where pub.dev want to see them for a better API doc coverage score. This is pretty good already, but you can easily get to 90% or higher:

As you know Flutter 2.5 comes with flutter_lints package used as default for new Flutter projects. The lint rules used here include all of the lint rules in flutter_lints, and many more, making this an even tougher lint rule set to comply by. But once it is done, using them and coding with them is imo no problem, the IDE tells you what to do 😃

Also you can use the dart fix --dry-run to test what lints need to be fixed and can be fixed automatically and then dart fix --apply to fix them, I already did that as part of this PR. It also takes care of deprecation API fixes, when possible, good tool to know: https://dart.dev/tools/dart-fix

If you use VS-Code you can set it to fix lints on save too.

I don't think it is available on IJ/AS yet, might come soon.

By the way my blog on linting has been updated to reflect Flutter 2.5 release and include updated rule comparison table between linters. https://rydmike.com/blog_flutter_linting

weta-vn / advance_image_picker