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

Conversation

desistefanova
Copy link
Contributor

@desistefanova desistefanova commented May 12, 2023

Notify when the log level is changed.
Logger onLevelChanged broadcasts a stream of level values.
Fixes dart-lang/core#479

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

Contribution guidelines:

Many Dart repos have a weekly cadence for reviewing PRs - please allow for a week or two of latency for initial review feedback.


@desistefanova desistefanova marked this pull request as ready for review May 12, 2023 17:41
@natebosch
Copy link
Contributor

@jakemac53 - any concerns about this API?

Copy link
Contributor Author

@desistefanova desistefanova left a comment

Choose a reason for hiding this comment

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

Thank you for the good suggestions! I pushed the code review changes.

@jakemac53
Copy link
Contributor

Looks like the pubspec version also needs to be updated to match the changelog 👍

@desistefanova desistefanova requested a review from jakemac53 May 18, 2023 05:29
@jakemac53 jakemac53 merged commit fa2486d into dart-archive:master May 18, 2023
@jakemac53
Copy link
Contributor

We can publish soon, once this rolls internally and we confirm there are no issues (which I can't imagine there would be, but we have a policy to do that).

@jakemac53
Copy link
Contributor

This is published now as version 1.2.0 @desistefanova

@kevmoo
Copy link
Contributor

kevmoo commented May 23, 2023

Beware adding things to types that are implemented! dart-lang/build#3514

@jakemac53
Copy link
Contributor

This type really isn't intended to be implemented... obviously it was but I am not sure what this package could do about that. We don't want to change our versioning strategy I don't think just because of one breakage (this is still overall much less pain than doing breaking changes every time we add something).

Really, build_runner_core should have had a tight constraint on this package if it was going to implement this class, so that is my bad.

@natebosch
Copy link
Contributor

This type really isn't intended to be implemented

Is it worth enforcing that? We could bump versions and add base.

@jakemac53
Copy link
Contributor

Is it worth enforcing that? We could bump versions and add base.

We would need to see if we can migrate off implementing it in build_runner_core first

@natebosch
Copy link
Contributor

I'm sure w can move to extends. I think we'd only have difficulty if we try to make the class final

@jakemac53
Copy link
Contributor

Yeah good point, sgtm

@natebosch
Copy link
Contributor

Ah, I was wrong 😞 , Logger also doesn't expose a generative constructor, so it doesn't make a difference if we use base or final...

@jakemac53
Copy link
Contributor

Ah yeah that is probably why we implemented it

mosuem pushed a commit to dart-lang/core that referenced this pull request Oct 16, 2024
Notify when the log level is changed. 
Logger `onLevelChanged` broadcasts a stream of level values.
Fixes dart-lang/logging#139
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.

Logger onLevelChanged notification needed

4 participants