Skip to content

Add many more global analyses.#47875

Merged
Hixie merged 2 commits intoflutter:masterfrom
Hixie:analysis
Dec 31, 2019
Merged

Add many more global analyses.#47875
Hixie merged 2 commits intoflutter:masterfrom
Hixie:analysis

Conversation

@Hixie
Copy link
Copy Markdown
Contributor

@Hixie Hixie commented Dec 27, 2019

  • Catch trailing spaces and trailing newlines in all text files.
    Before we were only checking newly added files, but that means we
    missed some.

  • Port the trailing spaces logic to work on Windows too.

  • Correct all the files with trailing spaces and newlines.

  • Refactor some of the dev/bots logic into a utils.dart library.
    Notably, the "exit" and "print" shims for testing are now usable
    from test.dart, analyze.dart, and run_command.dart.

  • Add an "exitWithError" function that prints the red lines and
    then exits. This is the preferred way to exit from test.dart,
    analyze.dart, and run_command.dart.

  • More consistency in the output of analyze.dart.

  • Check that all widgets have a "key" argument.

  • Add the "key" argument to the widgets that lacked them.

  • Refactor analyze.dart to use the _allFiles file enumerating logic
    more widely.

  • Add some double-checking logic to the _allFiles logic to catch
    cases where changes to that logic end up catching fewer files
    than expected (helps prevent future false positives).

  • Add a check to prevent new binary files from being added to
    the repository. Grandfather in the binaries that we've already
    added.

  • Update all the dependencies (needed because we now import crypto in
    dev/bots/analyze.dart).

@Hixie Hixie requested a review from jmagman as a code owner December 27, 2019 11:00
@fluttergithubbot fluttergithubbot added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. labels Dec 27, 2019
@fluttergithubbot fluttergithubbot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. tool Affects the "flutter" command-line tool. See also t: labels. labels Dec 27, 2019
@Hixie Hixie force-pushed the analysis branch 3 times, most recently from fe41106 to dd5ff63 Compare December 27, 2019 11:20
Comment thread dev/bots/analyze.dart Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: one line for this collection if

Comment thread dev/bots/analyze.dart Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: one line for this collection for

Comment thread dev/bots/analyze.dart Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a trailing \n here that is not L473 below

Comment thread dev/bots/analyze.dart Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to avoid \n it could be

...errors.expand((String paragraph) => <String>[paragraph, '']),

or

for (String paragraph in errors) ...<String>[
  paragraph,
  '',
],

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.

Strictly speaking I don't know which is better... I've left the \n for now.

@Hixie Hixie force-pushed the analysis branch 2 times, most recently from f9b41b5 to cec5047 Compare December 27, 2019 22:57
Comment thread .github/no-response.yml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we sure this doesn't break github's parsing of this file?

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.

no, not really. we'll find out pretty quick though. :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Intended? Will this not break the decryption?

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.

This is just a test file.

Comment thread dev/bots/analyze.dart Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't this O^6? Or is the compiler really able to fold these into a single test on one iteration?

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 would assume it's O(N), why would it be O(N^6)? Where is just a mapping iterator, it walks the incoming iterator, filters it, and only yields matching lines.

I don't think the compiler knows anything here, it's just how the iterator API is implemented.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, it's not O(N^6), but it's close to O(N*6) (a bit shy of that since each iteration eliminates at least one element) - each of those where clauses has to iterate the entire yielded set from the previous one, and those first few are only eliminating one or two files a piece.

You'd save a bunch of iterations if this was just a single where with a larger path.basename(file.path) != ... &&, or maybe even just !file.path.endsWith(...) && !file.path.endsWith...

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.

This code runs in milliseconds, I'm not really that concerned about making it faster, certainly not at the cost of readability. :-)

* Catch trailing spaces and trailing newlines in all text files.
  Before we were only checking newly added files, but that means we
  missed some.

* Port the trailing spaces logic to work on Windows too.

* Correct all the files with trailing spaces and newlines.

* Refactor some of the dev/bots logic into a utils.dart library.
  Notably, the "exit" and "print" shims for testing are now usable
  from test.dart, analyze.dart, and run_command.dart.

* Add an "exitWithError" function that prints the red lines and
  then exits. This is the preferred way to exit from test.dart,
  analyze.dart, and run_command.dart.

* More consistency in the output of analyze.dart.

* Refactor analyze.dart to use the _allFiles file enumerating logic
  more widely.

* Add some double-checking logic to the _allFiles logic to catch
  cases where changes to that logic end up catching fewer files
  than expected (helps prevent future false positives).

* Add a check to prevent new binary files from being added to
  the repository. Grandfather in the binaries that we've already
  added.

* Update all the dependencies (needed because we now import crypto in
  dev/bots/analyze.dart).
Copy link
Copy Markdown
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM modulo the stacked where clauses

@Hixie Hixie merged commit e768c92 into flutter:master Dec 31, 2019
@Hixie Hixie deleted the analysis branch December 31, 2019 01:12
franciscojma86 added a commit to franciscojma86/flutter that referenced this pull request Jan 2, 2020
franciscojma86 added a commit that referenced this pull request Jan 2, 2020
dnfield added a commit that referenced this pull request Jan 2, 2020
@a14n a14n mentioned this pull request Jan 16, 2020
9 tasks
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants