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

Migrate from deprecated pedantic to flutter_lints #519

Merged
merged 9 commits into from
Oct 6, 2021
Merged

Migrate from deprecated pedantic to flutter_lints #519

merged 9 commits into from
Oct 6, 2021

Conversation

adityathakurxd
Copy link
Contributor

@adityathakurxd adityathakurxd commented Oct 6, 2021

Description

Migrated all packages from deprecated pedantic to flutter_lints.
Added dev_dependency on package: flutter_lints to all project’s pubspec.yaml

Since the project already had a custom analysis_options.yaml file at its root, I added include: package:flutter_lints/flutter.yaml to it.

Related Issues

#471

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@miquelbeltran
Copy link
Member

Change looks good to me, need to finish the static analysis and see if it all works, then it can be approved and merged

@miquelbeltran miquelbeltran self-assigned this Oct 6, 2021
@miquelbeltran miquelbeltran added Hacktoberfest Issues taking part in Hacktoberfest hacktoberfest-accepted labels Oct 6, 2021
@miquelbeltran
Copy link
Member

As you can see @adityathakurxd there's a bunch of static analysis issues that came out since the flutter_lints is introduced, you would have to fix those before merging: https://github.com/fluttercommunity/plus_plugins/pull/519/checks?check_run_id=3811595912

@adityathakurxd
Copy link
Contributor Author

adityathakurxd commented Oct 6, 2021

Yes, I'll look into this @miquelbeltran Thank you for reviewing.

I wanted to ask like some of these messages in https://github.com/fluttercommunity/plus_plugins/pull/519/checks?check_run_id=3811595912 include Avoid print calls in production code but these statements are used to print(e.toString()); or other such cases. I wanted to ask if it would be right removing or should the fix be in analysis_option to allow print in production?

@miquelbeltran
Copy link
Member

I am OK with plugins using print() in case of errors, so we can add the rule to the analysis_options.yaml to ignore it.

I would also considering having a global analysis_options.yaml where we can define all the rules at the root of the project and linking to that, if that's possible!

@adityathakurxd
Copy link
Contributor Author

Okay. I'll add the rule. There is a global analysis_options.yaml at the root. I'll work with that.

@vbuberen
Copy link
Collaborator

vbuberen commented Oct 6, 2021

@miquelbeltran I would suggest switching from print to dart:developer as it is suggested here https://flutter.dev/docs/testing/code-debugging#logging
Or there is a specific reason to stick with print?

@miquelbeltran
Copy link
Member

@miquelbeltran I would suggest switching from print to dart:developer as it is suggested here https://flutter.dev/docs/testing/code-debugging#logging Or there is a specific reason to stick with print?

love the idea! I would suggest to do it on a separated PR after this one if possible.

@miquelbeltran
Copy link
Member

actually no need to wait for this PR ;)

@adityathakurxd
Copy link
Contributor Author

I have updated the PR with the required changes @miquelbeltran
For the print in production issue, to the global analysis_options.yaml I added:

 errors:
    avoid_print: ignore

and, the melos run analyze is running fine now.
Administrator_ Command Prompt - melos  run analyze 10_6_2021 4_09_19 PM (2)

@miquelbeltran
Copy link
Member

thank you @adityathakurxd seems that the analyze is passing I will merge once CI finishes

@adityathakurxd
Copy link
Contributor Author

Thank you so much @miquelbeltran 🎉

@adityathakurxd adityathakurxd removed their assignment Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Issues taking part in Hacktoberfest hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants