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

Remove custom logger #5

Conversation

batmat
Copy link
Contributor

@batmat batmat commented Sep 14, 2018

Somehow also following up on #1 (comment)

Though nice looking, I do not think that request-promise-retry should be configuring a custom logging.
Possibly, it could still keep a prefix for its logs, but IMO it should not define specific rules.

The application using this nice library is the place to customize logging.

First time I realized this and was surprising is when I had bumped the min log level to say warn, and was still seeing the retry related logs coming from here.

Thank you!

Though nice looking, I do not think that `request-promise-retry` should
be configuring a custom logging.
Possibly, it could still keep a prefix for its logs, but IMO it should
not define specific rules.

The application using this nice library is the place to customize logging.

First time I realized this and was surprising is when I had bumped the min
log level to say warn, and was still seeing the retry related logs coming
from here.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 25

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 98.246%

Totals Coverage Status
Change from base Build 22: -0.1%
Covered Lines: 37
Relevant Lines: 37

💛 - Coveralls

@batmat
Copy link
Contributor Author

batmat commented Sep 14, 2018

I would think @coveralls is wrong. How can removing a file lower the coverage? I think it's probably confused in its calculation by the reduction in number of lines and there's a bug there?

@batmat
Copy link
Contributor Author

batmat commented Sep 17, 2018

@void666 any chance to review and merge this? Thanks

@batmat batmat closed this Apr 15, 2022
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

2 participants