Turn off logging for testing calls. #3275
Conversation
PR HealthBreaking changes ✔️
Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
| } | ||
|
|
||
| var setLoggingOn = true; | ||
| assert(() { |
There was a problem hiding this comment.
What's the purpose of turning logging off when running in debug mode?
There was a problem hiding this comment.
It was your request to turn off logging for probes, right? So, if we turn off logging for ui in debug mode, propes and other dev related calls will not be logged.
Another option is to add a tag to such logs.
Did I miss something?
There was a problem hiding this comment.
That's correct, I think I understand why this is written this way. I just wanted to make sure that we are still able to log requests when running dart_services locally. This class is defined in dartpad_shared, but as long as it's only used in dartpad_ui this should be okay.
Maybe this class should be defined in dartpad_ui to avoid confusion?
| /// It can be set to false in constructor. | ||
| late final bool loggingOn; | ||
|
|
||
| DartPadRequestHeaders({bool? loggingOn}) { |
There was a problem hiding this comment.
nit: I would rename this to enableLogging to match other flags in the DartPad project (enableGenAi, enable-experiments, etc)
|
|
||
| import 'headers.dart'; | ||
|
|
||
| class DartPadBackendClient { |
There was a problem hiding this comment.
nit: rename to DartServicesClient for consistency
Contributes to #3270.
This is one option to turn off logging for probes.
I am open to brainstorm for better ones.
If looks good, I will extend it to other handlers and add test coverage.