Skip to content

Conversation

@pq
Copy link
Contributor

@pq pq commented Jun 3, 2016

Disables current package and current directory analysis when files are specified.

Fixes: #4091.

cc @Hixie

Disables current package and current directory analysis when files are specified.

Fixes: flutter#4091.
@abarth
Copy link
Contributor

abarth commented Jun 3, 2016

LGTM

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2016

This won't work because it would make us ignore the arguments if they're explicitly set.

@pq
Copy link
Contributor Author

pq commented Jun 3, 2016

it would make us ignore the arguments if they're explicitly set.

Agreed. I guess I was assuming that was not a common use case. Maybe I'm wrong though. Do you find yourself wanting to analyze a list of files and the current package or directory? I was seeing that as a degenerate case. Or?

@pq
Copy link
Contributor Author

pq commented Jun 3, 2016

@Hixie : added a check to ensure that we only override settings if they are not user-specified. PTAL.

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2016

I think I'd move the flutterRepo checks up into the top part as well, so that --current-directory --flutter-repo does both too. (Also maybe the docs should be adjusted to say that the default only applies if nothing else is specified on the command line.)

I agree that this is an edge case, but having this kind of polish throughout the entire project is what leads to people having confidence in the project. Everyone will hit some minor edge case. We have to get all of the edge cases right so that everyone's particular edge case works as they expect.

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2016

In any case, LGTM once you're confident you've polished it enough that it shines ;-)

@pq
Copy link
Contributor Author

pq commented Jun 3, 2016

having this kind of polish throughout the entire project is what leads to people having confidence in the project.

👍 I totally agree here.

Otherwise, I'm not totally sure what the desired behavior is WRT to flutter-repo. I think you mean

  • flutter analyze --current-directory --flutter-repo
  • flutter analyze --current-package --flutter-repo
  • flutter analyze --current-directory --current-package --flutter-repo

should analyze the repo, plus the directory, package and both respectively?

Presumably, this should add foo.dart too?

  • flutter analyze --current-directory --current-package --flutter-repo foo.dart

I'm inclined to tackle these variations with some tests in a follow-up PR after we clarify what variations we want to support (and how). Might be a good time to revisit the defaults too in the spirit of general polish.

Sound reasonable?

@Hixie
Copy link
Contributor

Hixie commented Jun 3, 2016

Sound reasonable?

Totally.

Maybe the right behavior is to have everything default off, and if you specify none, we use --current-package?

@pq
Copy link
Contributor Author

pq commented Jun 6, 2016

Cool. Opened #4387 to track command cleanup.

@pq pq merged commit 6d2d495 into flutter:master Jun 6, 2016
@pq pq deleted the fix_4091 branch August 11, 2016 17:31
cdotstout pushed a commit to cdotstout/flutter that referenced this pull request Apr 3, 2018
* Move texture registry ownership to platform view

This enables the texture registry to survive activity pause on Android.
cdotstout pushed a commit to cdotstout/flutter that referenced this pull request Apr 3, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"flutter analyze <foo>" should only analyze <foo>

3 participants