Skip to content

Unify Analysis Options YAML into a single reference file (see #74) #84

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
Merged
merged 11 commits into from
Dec 18, 2020
Merged

Unify Analysis Options YAML into a single reference file (see #74) #84

merged 11 commits into from
Dec 18, 2020

Conversation

miquelbeltran
Copy link
Member

@miquelbeltran miquelbeltran commented Dec 17, 2020 •
edited
Loading

This PR unifies all projects analysis_options.yaml into a single one and upgrades pedantic to 1.9.2, fixing all new analysis issues raised.

@miquelbeltran miquelbeltran changed the title WIP on #74 Unify Analysis Options YAML into a single reference file (see #74) Dec 17, 2020
@miquelbeltran
Copy link
Member Author

It's a massive PR, sorry, not a lot of lines changed but a lot of files.

@jpnurmi
Copy link
Member

jpnurmi commented Dec 17, 2020

It's a massive PR, sorry, not a lot of lines changed but a lot of files.

Could be done in steps one package at a time to make it easier to review? :)

@miquelbeltran
Copy link
Member Author

Maybe yes, in this case is because updating the version of pedantic on melos so it affected all packages. Some packages were at 1.8.0 while some others at 1.9.0.

@jpnurmi
Copy link
Member

jpnurmi commented Dec 18, 2020

Is the relative include from the repo root necessary? Would that break stand-alone packages downloaded from pub.dev?

P.S. Do you know how flutter/plugins does it? They seem to have one analysis_options.yaml in the repo root without any additional includes in the packages:

flutter/plugins (master)$ git grep analysis_options
analysis_options.yaml:include: package:pedantic/analysis_options.1.8.0.yaml
packages/video_player/video_player_web/CHANGELOG.md:* Add `analysis_options.yaml` to the package, so we can ignore `undefined_prefixed_name` errors. Works around https://github.com/flutter/flutter/issues/41563.
script/incremental_build.sh:# Plugins that deliberately use their own analysis_options.yaml.

@miquelbeltran
Copy link
Member Author

you are right, it is not necessary to have the file with the include except if you are specifying different rules, so I remove them all and just leave the root one

jpnurmi
jpnurmi approved these changes Dec 18, 2020
View reviewed changes
Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

It's a big PR but the changes are very simple. I glanced quickly through and have only some nit-pick. Fix if you feel like it. :)

packages/battery_plus/lib/battery_plus.dart
if (_singleton == null) {
_singleton = Battery._();
}
_singleton ??= Battery._();
Copy link
Member

Choose a reason for hiding this comment

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

👍 Nice, I've been tempted to fix these

packages/package_info_plus/example/analysis_options.yaml Outdated
@@ -0,0 +1 @@
include: ../../../analysis_options.yaml
Copy link
Member

Choose a reason for hiding this comment

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

can be removed?

packages/package_info_plus_macos/pubspec.yaml
Comment on lines -22 to -23
dev_dependencies:
pedantic: ^1.8.0
Copy link
Member

Choose a reason for hiding this comment

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

is this correct or should it be bumped to 1.9.2?

Copy link
Member Author

Choose a reason for hiding this comment

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

well spotted, actually there's not even dart code on the macos projects so I think the dependency could be removed

packages/package_info_plus_web/lib/package_info_plus_web.dart Outdated
Future<PackageInfoData> getAll() async {
String url =
"${Uri.parse(window.document.baseUri).removeFragment()}/version.json";
var url =
Copy link
Member

Choose a reason for hiding this comment

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

final?

packages/sensors_plus/example/lib/snake.dart Outdated
for (math.Point<int> p in state.body) {
final Offset a = Offset(cellSize * p.x, cellSize * p.y);
final Offset b = Offset(cellSize * (p.x + 1), cellSize * (p.y + 1));
for (var p in state.body) {
Copy link
Member

Choose a reason for hiding this comment

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

final?

@miquelbeltran
Copy link
Member Author

I am not sure why I got some analysis issues on my computer by they don't appear on CI, I created a ticket for that: #85

Besides, I am merging this now :)

@miquelbeltran
Copy link
Member Author

Thanks for reviewing @jpnurmi !

@miquelbeltran miquelbeltran merged commit 661738e into main Dec 18, 2020
@miquelbeltran miquelbeltran deleted the mb-unify-analysis-options branch December 18, 2020 10:10
@sureace sureace mentioned this pull request Jan 28, 2021
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jpnurmi jpnurmi jpnurmi 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.

2 participants

@miquelbeltran @jpnurmi