close
Skip to content

attempt to clean up DCM issues#9791

Open
srawlins wants to merge 1 commit intoflutter:masterfrom
srawlins:dcm
Open

attempt to clean up DCM issues#9791
srawlins wants to merge 1 commit intoflutter:masterfrom
srawlins:dcm

Conversation

@srawlins
Copy link
Copy Markdown
Contributor

No description provided.

@srawlins srawlins requested a review from a team as a code owner April 20, 2026 17:35
@srawlins srawlins requested review from bkonyi and removed request for a team April 20, 2026 17:35
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a linter ignore comment to suppress a warning regarding unnecessary type assertions when processing class names for autocompletion. The reviewer recommends avoiding the use of ignore comments by refactoring the code to use .whereType<String>(), which is a more idiomatic way to handle the collection and avoids the linter issue entirely.

Comment on lines 203 to 205
final classNames = classes.map((clazz) => clazz.name);
// ignore: avoid-unnecessary-type-assertions
result.addAll(classNames.nonNulls);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

[CONCERN] Avoid using // ignore comments to silence linter warnings. If DCM is incorrectly flagging nonNulls as an unnecessary type assertion, using whereType<String>() is a standard alternative that often avoids such false positives while remaining clear and null-safe. Inlining the operation also improves readability by removing a single-use local variable.

Suggested change
final classNames = classes.map((clazz) => clazz.name);
// ignore: avoid-unnecessary-type-assertions
result.addAll(classNames.nonNulls);
result.addAll(classes.map((clazz) => clazz.name).whereType<String>());
References
  1. Maintainability: Code should be easy to modify and extend. Consistency: Adhering to consistent style across all DevTools packages improves collaboration. (link)

if (classes != null) {
// Autocomplete class names as well
final classNames = classes.map((clazz) => clazz.name);
// ignore: avoid-unnecessary-type-assertions
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: add a reason after the ignore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or actually, is the lint firing because .nonNulls can be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

.nonNulls cannot be removed. I think it is a bug in DCM.

But this DCM CI run says that the ignore is unnecessary.

But I see on other CI that I just kicked off, we still have DCM issues: https://github.com/flutter/devtools/actions/runs/24530067465/job/72178946851?pr=9755

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you have the same version of DCM installed that we use on the CI? 1.36.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@incendial for the possible DCM bug

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not have DCM installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants