-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Checking for devtools config file for opt out #227
Checking for devtools config file for opt out #227
Conversation
@@ -137,13 +137,17 @@ Directory? getHomeDirectory(FileSystem fs) { | |||
/// Dart: `$HOME/.dart/dartdev.json` | |||
/// | |||
/// Flutter: `$HOME/.flutter` | |||
/// | |||
/// Devtools: `$HOME/.flutter-devtools/.devtools` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here "legacy" doesn't necessarily refer to the analytics instance it's going to, but an implementation of analytics that predates package:unified_analytics, right?
And therefore, we will need to maintain this code in perpetuity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't it mean both? Currently devtools has their own implementation for analytics while it also sends to a different GA instance.. is that what you mean?
And yes, we would maintain this. It only runs the first time package:unified_analytics
is run on a developers workstation though
This looks good, but I'll defer approval to Kenzie |
// reason, the file was written incorrectly | ||
return true; | ||
} on FileSystemException { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see this is the same logic for other tools with legacy opt in / opt out files, but why assume they are opted out if we can't read the file? Seems like instead we should assume that they haven't answered the question as to whether they want to opt in or not, and then we can ask them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair question, I think we wanted to approach this with a more conservative approach. I was imagining a developer who thought they were opted out in the past, somehow ended up with a corrupted file, and now gets prompted again... they may dismiss it thinking it was bug and now we're collecting their data
I'm not opposed to swapping out this logic, just wanted to highlight that edge case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that if a user opted out once, they'd opt out again when prompted. The user set I'd be worried about dropping are the users who have opted yes, and then somehow their file gets corrected, and now we opt them out instead of asking them again, which would show up as a drop in our usage that could be due to the logic here.
I would guess the corrupted file case is pretty rare though, so maybe this isn't as big of a risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would guess the corrupted file case is pretty rare though, so maybe this isn't as big of a risk.
I have a separate PR that is out that will allow us to track how many times we run into these catch blocks so that should help answer if this happens often in the wild
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
… mime, mockito, test, tools, vector_math Revisions updated by `dart tools/rev_sdk_deps.dart`. args (https://github.com/dart-lang/args/compare/46d5033..03386ba): 03386ba 2024-01-23 Albert Kaiser Add missing curly braces in README.md (dart-archive/args#261) csslib (https://github.com/dart-lang/csslib/compare/1ad2d1e..ec86ee5): ec86ee5 2024-01-09 Kevin Moore Require Dart 3.0, update and fix lints (dart-archive/csslib#194) ecosystem (https://github.com/dart-lang/ecosystem/compare/1e2785d..9ee08a4): 9ee08a4 2024-01-29 Moritz Add `ignore` flag to health workflows (dart-lang/ecosystem#218) a283d70 2024-01-17 Moritz Make health testable (dart-lang/ecosystem#224) f61a550 2024-01-16 Moritz Enable experiments for health (dart-lang/ecosystem#226) c81f25c 2024-01-16 Moritz Add submodule support to `publish.yaml` (dart-lang/ecosystem#225) b51c356 2024-01-12 Moritz Add submodules support to `health.yaml` (dart-lang/ecosystem#223) d7aaecb 2024-01-10 Moritz Run `health.yaml` for bots (dart-lang/ecosystem#222) 971c733 2024-01-10 Moritz Don't write the failure string when skipping (dart-lang/ecosystem#220) html (https://github.com/dart-lang/html/compare/06bc148..910f6d7): 910f6d7 2024-01-26 Kevin Moore Update lints, require Dart 3.2 (dart-archive/html#236) aaf7d1a 2024-01-25 Kevin Moore blast_repo fixes (dart-archive/html#235) http_multi_server (https://github.com/dart-lang/http_multi_server/compare/ae48489..491f7c6): 491f7c6 2024-01-24 Kevin Moore Update lints, require Dart 3.2 (dart-lang/http_multi_server#63) 0df95e0 2024-01-24 Kevin Moore blast_repo fixes (dart-lang/http_multi_server#62) logging (https://github.com/dart-lang/logging/compare/4d35a4e..e04942d): e04942d 2024-01-18 Kevin Moore update min SDK and deps (dart-archive/logging#155) a03a946 2024-01-18 Craig Labenz Hierarchical logging documentation (dart-archive/logging#146) 439ec80 2024-01-18 Kevin Moore blast_repo fixes (dart-archive/logging#154) mime (https://github.com/dart-lang/mime/compare/ca9f059..99fbdcc): 99fbdcc 2024-01-24 Kevin Moore Update to latest lints, require Dart 3.2 (dart-archive/mime#114) mockito (https://github.com/dart-lang/mockito/compare/e15e000..0422551): 0422551 2024-01-10 Oleh Prypin Ignore "must_be_immutable" warning in generated files. test (https://github.com/dart-lang/test/compare/846d73e..6700049): 6700049d 2024-01-29 Nate Bosch Prepare to publish package:checks (dart-lang/test#2178) a5c4f010 2024-01-24 Nate Bosch Use a raw string for console logging with path (dart-lang/test#2177) fe3102ee 2024-01-10 dependabot[bot] Bump js from 0.6.7 to 0.7.0 in /pkgs/test (dart-lang/test#2168) c709cde0 2024-01-10 Jacob MacDonald fix a bug where test html files were not created in precompiled mode (dart-lang/test#2170) 0eddae47 2024-01-09 Nate Bosch Document the silent reporter (dart-lang/test#2163) tools (https://github.com/dart-lang/tools/compare/8ffc077..f6e67f2): f6e67f2 2024-01-29 Elias Yishak Fix logic for first run to fix `shouldShowMessage` behavior (dart-lang/tools#228) b97bd5c 2024-01-29 Elias Yishak Checking for devtools config file for opt out (dart-lang/tools#227) vector_math (https://github.com/google/vector_math.dart/compare/38a00c3..cb976c7): cb976c7 2024-01-28 Andrew Brampton Update README.md to show how to use vector_math_64. (google/vector_math.dart#312) d99c903 2024-01-25 Andrew Brampton Added a toString, operator == and hashCode to the Quad class. (google/vector_math.dart#311) Change-Id: Ie42ec078b7b4d408d5167e38f05f1f37b754afb0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/349301 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Devon Carew <devoncarew@google.com>
This PR will make sure that when the config file for
package:unified_analytics
is being created the first time, it will check in the devtools config file for previous opt out status. If the user has already opted out in the past but haven't interacted withpackage:unified_analytics
yet, then it will pass that information and opt out the user frompackage:unified_analytics
collection.Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.