Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Mar 26, 2020

Starting with a minimal set of lints and hints. Will gradually expand this file until it's no longer necessary and we can use the global repository analysis_options.yaml.

@yjbanov yjbanov added the Work in progress (WIP) Not ready (yet) for review! label Mar 26, 2020
@auto-assign auto-assign bot requested a review from iskakaushik March 26, 2020 00:06
@yjbanov yjbanov requested review from ferhatb and mdebbar and removed request for iskakaushik March 26, 2020 16:38
@yjbanov yjbanov removed the Work in progress (WIP) Not ready (yet) for review! label Mar 26, 2020
width: width,
lineNumber: lineNumber,
left: left,
// TODO(yjbanov): I chose -1 value just to fix the analyzer warning. I don't know if it's the right value to pass.
Copy link
Contributor

Choose a reason for hiding this comment

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

The default is -1. No need for TODO. /cc @mdebbar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

I would love to be fixing these issues in the files that I'm touching anyway. Is there a way we could enable the analyzer options locally, but not in cirrus?

LGTM

Comment on lines 1157 to 1158
// TODO(yjbanov): I chose -1 value just to fix the analyzer warning. I don't know if it's the right value to pass.
endIndexWithoutNewlines: -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any value is fine here since we don't use it for comparison in tests.

@yjbanov
Copy link
Contributor Author

yjbanov commented Mar 26, 2020

I would love to be fixing these issues in the files that I'm touching anyway. Is there a way we could enable the analyzer options locally, but not in cirrus?

Yes, VSCode and IntelliJ pick up this file automatically, so all the lints will show up in your editor as you type. I cleaned up all code for these particular hints, so they should never come back.

@mdebbar
Copy link
Contributor

mdebbar commented Mar 26, 2020

I meant the commented ones.

@yjbanov
Copy link
Contributor Author

yjbanov commented Mar 26, 2020

Yeah, you can uncomment them but not commit the analysis_options.yaml changes.

@yjbanov
Copy link
Contributor Author

yjbanov commented Mar 26, 2020

luci-engine failure is Fuchsia infra flakiness. Landing.

@yjbanov yjbanov merged commit 5389159 into flutter:master Mar 26, 2020
chinmaygarde added a commit that referenced this pull request Mar 27, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 27, 2020
chinmaygarde added a commit that referenced this pull request Mar 27, 2020
yjbanov added a commit to yjbanov/engine that referenced this pull request Mar 27, 2020
This time I'm starting without Cirrus. Will add Cirrus serpartely from code changes.

This reverts commit 6d33ee1.
yjbanov added a commit that referenced this pull request Mar 27, 2020
* Reland "add limited analysis options (#17332)"

This time I'm starting without Cirrus. Will add Cirrus serpartely from code changes.

This reverts commit 6d33ee1.

* disable Cirrus analysis check
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants