Skip to content
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

v0.9.2 #52

Closed
wants to merge 11 commits into from
Closed

v0.9.2 #52

wants to merge 11 commits into from

Conversation

gmpassos
Copy link
Contributor

  • New MultiOutput: allow multiple simultaneous outputs.
  • New AsyncOutput: performs output in asynchronous mode.
  • New LogPlatform: identifies the runtime platform, and it's capabilities.
  • New LoggerFactory: A Logger factory with predefined properties.
  • ConsoleOutput: now merging multiple lines to one String to avoid multiple IO calls.
  • MemoryOutput: overrides [] operator for events buffer access.
  • SimplePrinter, PrettyPrinter, PrefixPrinter: added optional name property.
  • Logger.log(): try catch while sending events to output,
    to avoid Logger influence over the main software.
  • README.md: fixed and improved badges.

- New MultiOutput: allow multiple simultaneous outputs.
- New AsyncOutput: performs output in asynchronous mode.
- New LogPlatform: identifies the runtime platform, and it's capabilities.
- New LoggerFactory: A `Logger` factory with predefined properties.
- ConsoleOutput: now merging multiple lines to one String to avoid multiple IO calls.
- MemoryOutput: overrides `[]` operator for events buffer access.
- SimplePrinter, PrettyPrinter, PrefixPrinter: added optional `name` property.
- Logger.log(): `try` `catch` while sending events to output,\
  to avoid `Logger` influence over the main software.
- README.md: fixed and improved badges.
…eEpoch`, since `_maximumFlushTime` is in milliseconds.
@ngoctranfire
Copy link

I think this should also solve most of the cases I had presented in #54. This is a great help. I'm curious, but have we thought about consolidating our efforts with lumberdash? I feel like both libraries have a lot of strengths, but this one can really start to build different "clients", kind of how lumberdash has them too:
https://github.com/jorgecoca/lumberdash

@gmpassos
Copy link
Contributor Author

gmpassos commented Jun 26, 2020 via email

@ngoctranfire
Copy link

Do you know what the status of this seems to be right now? I really think this should be updated to push in as soon as possible. @leisim any status updates?

@haarts
Copy link
Collaborator

haarts commented Jun 29, 2020

I'll be able to give this some love on Thursday. Work prevented me from looking into this earlier.

Copy link
Collaborator

@haarts haarts left a comment

Choose a reason for hiding this comment

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

First off: thank you so much for your contributions. There's a lot to love in here!

Second, I haven't completely finished the review, the PrettyPrinter I have really looked at. That needs to come later, hopefully tomorrow.

Also I really dig what you have done with the whole platform thing. That was certainly clunky previously. But I do feel a bit uncomfortable with the tight coupling with the ConsoleOutput. But I have not ready answer on how to address that.

Please have a look at the comments I've made so far. We'll work together to get this in!

CHANGELOG.md Show resolved Hide resolved
lib/src/printers/pretty_printer.dart Show resolved Hide resolved
lib/src/log_factory.dart Outdated Show resolved Hide resolved
lib/src/log_printer.dart Show resolved Hide resolved
lib/src/logger.dart Show resolved Hide resolved
lib/src/platform/platform.dart Show resolved Hide resolved
lib/src/platform/platform.dart Show resolved Hide resolved
lib/src/platform/platform.dart Outdated Show resolved Hide resolved
lib/src/platform/platform.dart Outdated Show resolved Hide resolved
lib/src/printers/pretty_printer.dart Show resolved Hide resolved
@haarts
Copy link
Collaborator

haarts commented Jul 10, 2020

I think the platform business is close to be good to merge. Some of that code belongs to the ConsoleOutput I think but I feel that is of minor importance.

The named logger thing I agree with too.

The AsyncOutput and LoggerFactory I'm not convinced off. I would rather have those in a separate pull request.

The changes to the MemoryOutput seem redundant and I would rather keep the code base small.

The bug fixes are obviously good.

@gmpassos
Copy link
Contributor Author

The AsyncOutput and LoggerFactory I'm not convinced off. I would rather have those in a separate pull request.

  • AsyncOutput:
    important for applications with high volume of requests, like a backend.

  • LoggerFactory:
    I just implemented the basics to have something similar to http://www.slf4j.org/, a very famous logging framework for Java.
    For me this is very important, since I will be able to have different logger configurations depending of the namespace/package and dynamic configurations (like a development mode without change the code).

@haarts
Copy link
Collaborator

haarts commented Aug 17, 2020

This pull request has been lingering for some time now. The blame is mine.
I want to suggest the following;

  • I see little additional benefit for the AsyncLogger and suggest you keep it in your own code base.
  • Same goes with the LoggerFactory. I'm really trying to keep the code base small and with maximum impact.
  • I will merge the pull request after these are removed (or I can do that when I merge). After the merge I'll take a couple of hours to the merged code with the remainder of the library. This probably means that not everything is going to work as you originally envisioned!

This is a bit in line with what I said here.

There is a lot of good stuff in the PR and there's also just a lot. I do want to land this valuable contribution!

@gmpassos
Copy link
Contributor Author

I have made the requested changes

@haarts
Copy link
Collaborator

haarts commented Sep 24, 2020

A whole bunch of this stuff just landed on master. What's notably missing is named loggers. I thought that perhaps we could use the PrefixPrinter for that. Keep it nice and composable. The PrettyPrinter is also kept simple as opposed to the suggestion here. The proposed new code lacked test coverage and I was unsure why this functionality would be beneficial to the logger package.

I'm closing this PR and ponder on the named loggers.

@haarts haarts closed this Sep 24, 2020
@gmpassos
Copy link
Contributor Author

The main idea of a named logger is to show (when printing) the origin of the log event. In Java usually is the name of the class or a package. Best regards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants