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

Feature/additional loggers #7

Merged
merged 14 commits into from
Dec 29, 2021
Merged

Conversation

bartekpacia
Copy link
Contributor

@bartekpacia bartekpacia commented Dec 10, 2021

fix #5 (see it for discussion)

@bartekpacia bartekpacia requested review from Albert221, shilangyu and a user December 10, 2021 11:42
@bartekpacia bartekpacia force-pushed the feature/additional_loggers branch from 8f19110 to 5735404 Compare December 10, 2021 11:46
import 'logging_bugfender.dart';

/// Defines how [LoggingBugfenderListener] prints.
abstract class PrintStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont get it, why is it an interface and not just an enum?

Copy link
Member

Choose a reason for hiding this comment

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

This strategy should have an abstract method that we actually can have various strategies on - creating the String log message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I thought of adding some config options, for example to ColoredTextPrintStrategy. But yeah, this should be an enum.

Copy link
Member

Choose a reason for hiding this comment

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

No. Refer to my comment below and what strategies are. :)

Copy link
Contributor Author

@bartekpacia bartekpacia Dec 10, 2021

Choose a reason for hiding this comment

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

I added my comment while yours @Albert221 was not visible. Sorry :) I totally agree.

lib/src/logging_bugfender.dart Outdated Show resolved Hide resolved
import 'logging_bugfender.dart';

/// Defines how [LoggingBugfenderListener] prints.
abstract class PrintStrategy {
Copy link
Member

Choose a reason for hiding this comment

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

This strategy should have an abstract method that we actually can have various strategies on - creating the String log message.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Dec 23, 2021

Somehow I can't get ASCII colors to work in VSCode terminal. TBH I'm a bit tired of it.

I think that we can merge the current, basic version with 2 print strategies (NeverPrintStrategy and PlainTextPrintStrategy), and add the third one ColoredTextPrintStrategy in the future (see #8). Done is better than perfect.

@Albert221 @shilangyu what do you think? I'd appreciate a review.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@shilangyu shilangyu left a comment

Choose a reason for hiding this comment

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

LGTM

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Dec 25, 2021

@Albert221 it still says "changes requested" :) I'd appreciate your review

Copy link
Member

@Albert221 Albert221 left a comment

Choose a reason for hiding this comment

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

LGTM, with few documentation remarks

lib/src/logging_bugfender.dart Outdated Show resolved Hide resolved
lib/src/logging_bugfender.dart Outdated Show resolved Hide resolved
@bartekpacia bartekpacia merged commit b56ccdf into master Dec 29, 2021
@bartekpacia bartekpacia deleted the feature/additional_loggers branch December 29, 2021 11:30
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.

Add common code
3 participants