fix!: Make `melos analyze` always use `dart analyze`

invertase / melos

🌋 A tool for managing Dart projects with multiple packages. With IntelliJ and Vscode IDE support. Supports automated versioning, changelogs & publishing via Conventional Commits.

https://melos.invertase.dev/~melos-latest

Apache License 2.0

1.07k stars 193 forks source link

fix!: Make `melos analyze` always use `dart analyze` #695

Closed Pavel-Sulimau closed 2 months ago

Pavel-Sulimau commented 2 months ago

Description

The recent melos analyze changes came as a breaking change surprise for our project. On our project, we do not treat info-level issues as errors because we have some deprecated info lints in our codebase.

The change introduced in https://github.com/invertase/melos/pull/655 results in the call to flutter analyze or dart analyze depending on the Flutter dependency presence.

The issue is that those commands have different default behavior and configuration options.

While dart analyze does not fail on info-level issues by default,

the flutter analyze does fail on info-level issues.

This results in inconsistent behavior. Also, it is not aligned with the current documentation/API of melos analyze.

Since dart analyze is suitable for both dart and flutter packages I do not see a problem using just this command. That will fix the inconsistency and make things simpler.

Type of Change

[ ] ✨ feat -- New feature (non-breaking change which adds functionality)
[x] 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
[x] ❌ ! -- Breaking change (fix or feature that would cause existing functionality to change)
[ ] 🧹 refactor -- Code refactor
[ ] ✅ ci -- Build configuration change
[x] 📝 docs -- Documentation
[ ] 🗑️ chore -- Chore

spydon commented 2 months ago

Hmm, to me it sounds more consistent to make analyze behave like Flutter if it is a Flutter project and like Dart if it is a Dart project.

Pavel-Sulimau commented 2 months ago

Hmm, to me it sounds more consistent to make analyze behave like Flutter if it is a Flutter project and like Dart if it is a Dart project.

@spydon, I've got a couple of questions then:

How could I make the melos analyze in the current version of Melos (5.3.0) not fail on info-level issues on a Flutter (not pure Dart) package taking into account there is no option to pass --no-fatal-infos?
Which advantages of using flutter analyze over dart analyze do you see nowadays?

P.S. Some additional info on the topic:

spydon commented 2 months ago

How could I make the melos analyze in the current version of Melos (5.3.0) not fail on info-level issues on a Flutter (not pure Dart) package taking into account there is no option to pass --no-fatal-infos?

Ah yes, that should definitely be added as a flag to Melos!

Which advantages of using flutter analyze over dart analyze do you see nowadays?

No advantages at all, but we shouldn't dictate from Melos side that the dart analyzer presets are used in a Flutter project, when people with Flutter projects will be expecting it to go through the Flutter analyzer presets. I hope that in the future the analyzer presets won't differ between Dart and Flutter.

Adding the --no-fatal-infos flag would also be better since it wouldn't be a breaking change.

spydon commented 2 months ago

@Pavel-Sulimau do you want to work on adding the --no-fatal-infos flag instead? That should solve your problem too, right?

Pavel-Sulimau commented 2 months ago

Adding the --no-fatal-infos flag would also be better since it wouldn't be a breaking change.

This breaking change does not scare me as much as the current way of working of melos analyze :)

@Pavel-Sulimau do you want to work on adding the --no-fatal-infos flag instead? That should solve your problem too, right?

@spydon, Yep, sure, I can do that! However, I would like to double-check on your suggestions as at the moment it seems to me suboptimal, i.e. adding unnecessary complexity/confusion without explicit value. Here is what is bothering me a bit about that:

The dart analyze and flutter analyze presets are different, so we'll have to ignore --no-fatal-infos for the pure Dart projects as otherwise the dart analyze command will fail. IMO that will be unintuitive if the melos analyze is going to provide such an option. We'll also need to adjust https://melos.invertase.dev/commands/analyze to elobarate that not only dart analyze is used under that hood.
Flutter repository itself already uses dart analyze for Flutter projects, some other well-known packages like flutter_bloc (see this) and plus_plugins (see this) do that as well.

So, could you please confirm that using both flutter analyze and dart analyze and finding the common denominator between their presets for melos analyze is the preferred way to go?

spydon commented 2 months ago

1. The `dart analyze` and `flutter analyze` presets are different, so we'll have to ignore `--no-fatal-infos` for the pure Dart projects as otherwise the `dart analyze` command will fail. IMO that will be unintuitive if the `melos analyze` is going to provide such an option.  We'll also need to adjust https://melos.invertase.dev/commands/analyze to elobarate that not only `dart analyze` is used under that hood.

We just don't add --fatal-infos for dart projects when --no-fatal-infos is present, so it will behave like one would expect.

2. [Flutter repository itself already uses](https://github.com/flutter/samples/blob/f4083c19b21d723cc9973eed24d289e1022b5aa2/tool/flutter_ci_script_shared.sh#L15C9-L15C21) `dart analyze` for Flutter projects, some other well-known packages like flutter_bloc ([see this](https://github.com/felangel/bloc/blob/45591078de7e0d5d3b03d4488e42c4be020ae738/.github/actions/flutter_package/action.yaml#L64C12-L64C24)) and plus_plugins ([see this](https://github.com/fluttercommunity/plus_plugins/blob/15985a1d1283122826aeff071473aed06ccf0265/.github/workflows/all_plugins.yaml#L37C39-L37C52)) do that as well.

So, could you please confirm that using both flutter analyze and dart analyze and finding the common denominator between their presets for melos analyze is the preferred way to go?

Yeah, there are definitely flutter projects using the Dart analyzer too.

I have a feeling that the big majority is using flutter analyze for their Flutter apps though, but if that is not the case, I think we can go your way and just just dart analyze. I made a little poll here to check, so hopefully we get a lot of data on that one.

spydon commented 2 months ago

We had a chat internally, and people are for just using dart analyze. And also, the poll came out on like 40/60 (with 100 participants), so there wasn't really a clear "winner" there anyways. So let's go ahead with this PR! I guess we'll have to make it a breaking change though, since it is will change the current behavior for Flutter projects.

Pavel-Sulimau commented 2 months ago

We had a chat internally, and people are for just using dart analyze. And also, the poll came out on like 40/60 (with 100 participants), so there wasn't really a clear "winner" there anyways. So let's go ahead with this PR! I guess we'll have to make it a breaking change though, since it is will change the current behavior for Flutter projects.

All right, thanks for reaching out to people Lukas! Let me know then if there is anything else I need to improve/fix in the current PR.

spydon commented 2 months ago

All right, thanks for reaching out to people Lukas! Let me know then if there is anything else I need to improve/fix in the current PR.

I think it looks good as it is, I'll go ahead and merge it now. :) I think we'll wait a bit with making a release to see if there is anything more coming in with potential breaking changes.