SourceHorizon / logger

Small, easy to use and extensible logger which prints beautiful logs.
https://pub.dev/packages/logger
MIT License
197 stars 33 forks source link

Allow switching out default DevelopmentFilter globally #33

Closed yangsfang closed 1 year ago

yangsfang commented 1 year ago

Asserts are expensive per Flutter doc

It is better to use kDebugMode when using Flutter (which I believe will be true for a majority of the user). Since kDebugMode is a constant, the compiler can optimize away the conditional check making it much faster than assert().

But since this project is dart only, we cannot introduce a dependency on package:flutter/foundation.dart because it will break the minority of users who are dart only. So it would be the best to allow the user to switch out the implementation of the DevelopmentFilter.

While currently it is already possible to pass filter to every Logger() call, it is much cleaner if this can be done globally.

The proposal is to allow switching out the implementation similar to Logger.level.

Bungeefan commented 1 year ago

Hi, thanks for opening this issue, however, I think you misinterpreted the Flutter docs you linked.

The documentation says the following about asserts (in Flutter):

The overlay should always be viewed in profile mode, since debug mode performance is intentionally sacrificed in exchange for expensive asserts that are intended to aid development, and thus the results are misleading.

Debug mode enables additional checks (such as asserts) that don’t run in profile or release builds, and these checks can be expensive.

As far as my understanding goes, they are, for example, talking about the extensive layout checks that are performed in asserts in Flutter code, however, this does not mean that asserts are per se expensive, just that Flutter uses them to do expensive work sometimes.

And for my second point, asserts are completely ignored and optimized out from release code, therefore, any performance penalty vanishes, even when using our DevelopmentFilter! Dart doc about asserts:

In production code, assertions are ignored, and the arguments to assert aren’t evaluated.

Besides all that, even if I believe that the initial reasoning is incorrect, I am willing to accept your PR to allow changing the default filter via a static field beforehand.

yangsfang commented 1 year ago

I'm not sure if asserts are expensive sometimes or all the time, but I can agree that they are optimized out in release mode and profile mode. So we are only really talking about debug mode performance.

Normally performance isn't really a big concern in debug mode, so I'm not super tied to this micro-optimization either. (Under debug mode, there is indeed a 1-second difference when I run it 100,000,000 times in a loop, but no one would really do that for real).

And also I thought about it more: it is possible to use factory/static methods to create the logger without repeating it in every constructor as I originally stipulated.

Logger createLogger({
  LogFilter? filter,
  LogPrinter? printer,
  LogOutput? output,
  Level? level,
}) {
  return Logger(
    filter: filter ?? CustomDevelopmentFilter(),
    printer: printer,
    output: output,
    level: level,
  );
}

So it is really up to you on whether you think this adds value in allowing people to customize the default filter vs having people write their own factory method.

If you do decide to merge it, I don't see any conflict at the moment.