packages
packages copied to clipboard
flutter
[camera] Ignore implementation imports outside of lib
Work towards https://github.com/dart-lang/linter/issues/4859
There are libraries outside a lib/ directory, which violate implementation_imports. The fix here is to always add an 'ignore' exception, as the "implementation" being imported is the very package being exemplified.
(Maybe that's a perfect reason to re-examine the public API of the package; can you create a good example / example test without importing the private implementation libraries?)
Pre-launch Checklist
- [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
- [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
- [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use
dart format.) - [x] I signed the CLA.
- [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g.
[shared_preferences] - [x] I linked to at least one issue that this PR fixes in the description above.
- [x] I updated
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes. - [ ] I updated
CHANGELOG.mdto add a description of the change, following repository CHANGELOG style. - [ ] I updated/added relevant documentation (doc comments with
///). - [x] I added new tests to check the change I am making, or this PR is test-exempt.
- [x] All existing and new tests are passing.
If you need help, consider asking for advice on the #hackers-new channel on Discord.
I have many more PRs like this to mail; please let me know how I can proceed; I can keep just adding // ignore: and // ignore_for_file: comments. I can put comments at the end of the line, above all imports, whatever. LMK.
@srawlins if you're finding these inside integration_tests of web packages, I think you should be able to just //ignore_for_file. The analyzer doesn't understand that these tests belong to the parent package, but we need an example app to flutter drive them (so they look like bad imports).
I'm not sure that redoing the exposed API of the package to expose things "visibleForTesting" is worth it (but we could have a top-level library that reexports what we need just for testing, I guess? It could be done, the implementation packages are private for the users' of a plugin, so they should not be importing from these APIs directly)
If you find any of these outside of integration_tests, those probably do need revisiting, though!
WDYT @stuartmorgan?
Yes, there's a very specific pattern for plugins in particular, where the only way to do end-to-end testing of even most individual methods is to run it in the integration_test of an app (because the native code that is half of the code under test doesn't exist in Dart unit tests or widget tests), so we do a lot of what is functionally more like unit testing of the package in example/integration_test instead.
So I agree that we should just treat anything in example/integration_test/ of a plugin just like we would test/, and then look closely at anything else (if there are other things) that violate the lint.
Thanks @srawlins! Also, if you want to make one big PR with all the changes, I'll get it reviewed and merged as soon as I see it! (or maybe I can create the super big PR if you want)
Here is the file with all of the lines that need changing (~60) across this repo: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8756626596148228113/+/u/analyze_flutter_packages/stdout?format=raw
WDYT about fixing them? I was going to do it more incrementally, and on the back burner. But I'd love if you wanted to clean them up 😁
@stuartmorgan Do you mean these lines?
info - test/legacy/webview_flutter_test.dart:7:8 - Import of a library in the 'lib/src' directory of another package. Try importing a public library that exports this library, or removing the import. - implementation_imports
info - test/legacy/webview_flutter_test.dart:8:8 - Import of a library in the 'lib/src' directory of another package. Try importing a public library that exports this library, or removing the import. - implementation_imports
info - test/legacy/webview_flutter_test.dart:14:8 - Import of a library in the 'lib/src' directory of another package. Try importing a public library that exports this library, or removing the import. - implementation_imports
These correspond to these source lines:
import 'package:flutter/src/foundation/basic_types.dart';
import 'package:flutter/src/gestures/recognizer.dart';
...
import 'package:webview_flutter/src/webview_flutter_legacy.dart';
Those imports are considered "implementation imports." Should test/legacy be considered differently?
Is test/ not exempt from this lint? I would have thought unit tests importing from src/ would be pretty common.
Oh I see you mean the third import there. That does seem odd. I can look into that.
It seems the implementation_imports lint is already enabled:
- https://github.com/flutter/packages/blob/main/analysis_options.yaml#L102
But my analyzer is not picking any of those up:
dit@dit:/work/flutter/packages/packages/camera/camera_web/example/integration_test$ flutter analyze .
Analyzing integration_test...
No issues found! (ran in 2.8s)
dit@dit:/work/flutter/packages/packages/camera/camera_web/example/integration_test$ dart analyze .
Analyzing .... 1.3s
No issues found!
How can I modify our analysis_options.yaml so I can generate my list of affected files?
I think it would be quite a bit of effort, to re-create the fix for https://github.com/dart-lang/linter/issues/4859 in a custom Dart SDK and use that to analyze this repo. The only way I know how is with the bot that generated that report. 😁
Ahhh oof, didn't realize this was a change in the behavior of the lint itself!
(PS: will try again tomorrow, it's a little bit late now in PT!)
My bad, the third violating import is this one:
import 'package:webview_flutter_platform_interface/src/webview_flutter_platform_interface_legacy.dart';
line-off-by-one human-error 😁
Thanks, that makes more sense. That one we can ignore since it's a very weird special case (upstream testing of something that's a use case that's only allowed within google3).
The flutter includes seem weird; we'll need to look into those.