Skip to content
This repository has been archived by the owner on Oct 18, 2024. It is now read-only.

Fix hierarchical logging documentation #171

Closed
wants to merge 1 commit into from

Conversation

AhmedLSayed9
Copy link

@AhmedLSayed9 AhmedLSayed9 commented Jul 28, 2024

The hierarchical logging documentation is inaccurate and needs to be updated.
Note: Logging level should enable logging for all levels above that level not below that level.

  • Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@kevmoo kevmoo requested a review from devoncarew July 29, 2024 02:44
@devoncarew
Copy link
Contributor

Thanks for the contribution. I think the existing example is too long and seems to be trying to demonstrate a few different things. I think is should be shortened quite a bit in order to just show very simple API usage. This might mean in-lining the sample from the example/ dir, or perhaps just eliding the code sample in the Configuration section completely.

The existing example does look correct however - both the comments in the code and the output as quoted.

@AhmedLSayed9
Copy link
Author

Thanks for the contribution. I think the existing example is too long and seems to be trying to demonstrate a few different things. I think is should be shortened quite a bit in order to just show very simple API usage. This might mean in-lining the sample from the example/ dir, or perhaps just eliding the code sample in the Configuration section completely.

The existing example does look correct however - both the comments in the code and the output as quoted.

I don't think it's all correct. I've simplified and updated the changes to clarify more about the incorrect behavior/output.
I'll explain them one by one in the following comments.

@@ -108,34 +108,30 @@ their `onRecord` streams.
});


// Will NOT print because FINER is too low level for `Logger.root`.
// Will NOT print because FINER is too low level for ([LOG1] & [ROOT]).
log1.finer('LOG_01 FINER (X)');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is correct, I've just updated the comment to be more clear.

log1.finer('LOG_01 FINER (X)');

// Will print twice ([LOG1] & [ROOT])
log1.fine('LOG_01 FINE (√√)');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is incorrect.
It should print once by [LOG1] only as the root logger has higher level WARNING.
As per Level definiton, logging level should enable logging only for all levels above the level not below it.

However, the output shows that the root logger fires which is actually a bug https://github.com/dart-lang/logging/issues/145


// Will print ONCE because `log1` only uses root listener.
log1.warning('LOG_01 WARNING (√)');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is incorrect.
It should print twice as WARNING is sufficient for both [LOG1] & [ROOT], and the output actually print it twice as:

[LOG1][FINE+] LOG_01 WARNING (√)
[ROOT][WARNING+] LOG_01 WARNING (√)


// Will never print because FINE is too low level.
// Will NOT print because FINE is too low level for ([LOG1] & [ROOT]).
log2.fine('LOG_02 FINE (X)');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is correct, I've just updated the comment to be more clear.


// Will never print because FINE is too low level.
// Will NOT print because FINE is too low level for ([LOG1] & [ROOT]).
log2.fine('LOG_02 FINE (X)');

// Will print twice ([LOG2] & [ROOT]) because warning is sufficient for all
// loggers' levels.
log2.warning('LOG_02 WARNING (√√)');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is correct.


// Will never print because `info` is filtered by `Logger.root.level` of
// `Level.WARNING`.
log2.info('INFO (X)');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is correct but it's not needed as it's the same case as log2.fine('LOG_02 FINE (X)');

@mosuem
Copy link
Contributor

mosuem commented Oct 17, 2024

Closing as the dart-lang/logging repository is merged into the dart-lang/core monorepo. Please re-open this PR there!

@mosuem mosuem closed this Oct 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants