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

feat: Use flutter analyze --no-fatal-infos for more sensible settings #262

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

romanr
Copy link

@romanr romanr commented Oct 15, 2024

Disable failing on info analysis output as info means suggestions and does not necessarily mean productive feedback. Discussed here.

Status

READY]

Description

Type of Change

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

@romanr romanr requested a review from a team as a code owner October 15, 2024 14:54
@tomarra tomarra changed the title Use flutter analyze --no-fatal-infos for more sensible settings feat: Use flutter analyze --no-fatal-infos for more sensible settings Oct 15, 2024
@tomarra tomarra added the p2 Important issues not at the top of the work list label Oct 22, 2024
Copy link
Contributor

@tomarra tomarra left a comment

Choose a reason for hiding this comment

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

Not sure if we want this change overall. At a minimum this should be a flag that should default to true (given that its the current functionality) and possibly let someone turn it off if they need to.

That being said the best practice here would actually be to setup an analysis_options.yaml to actually exclude the specific warnings you want to skip or put those directives in the problematic files themselves.

Change to `analyze options` instead of specific parameter
Copy link
Contributor

@tomarra tomarra left a comment

Choose a reason for hiding this comment

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

After talking with the team on this we think the best path forward for this is actually having a distinct flag for each parameter you want to enable rather then an open placeholder. This would allow for more configurability long term as well as a clear understanding of exactly what flags are getting turned on and off at a given time.

The other piece of feedback is that we are looking for a use case or reasoning behind the need for this change. Typically analyzer changes are done via the analysis_options.yaml file instead of tool flags so all the configuration is done in one spot. Is that not possible for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 Important issues not at the top of the work list
Projects
Status: Community
Development

Successfully merging this pull request may close these issues.

2 participants