search

trustpilot / node-trustpilot

HTTP client for Trustpilot
MIT License
32 stars 4 forks source link

Switch to eslint #36

Closed TPRobots closed 7 years ago

TPRobots commented 7 years ago

Pull request opened by github-pullrequestcreator.

miklosaubert commented 7 years ago

Oh, I took that from another NodeJS project, and to be honest, I don't remember the rationale for it. I guess it's based on this: http://eslint.org/docs/rules/no-console#when-not-to-use-it. I'm happy to remove it if it's a concern.

On 3 April 2017 at 08:41, Bardur Pihl notifications@github.com wrote:

@b-dur commented on this pull request.

In .eslintrc https://github.com/trustpilot/node-trustpilot/pull/36#discussion_r109347740 :

@@ -0,0 +1,6 @@ +{

  • "extends": "trustpilot",
  • "rules": {
  • "no-console": "off"

Sure you want to enable console statements?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/trustpilot/node-trustpilot/pull/36#pullrequestreview-30442826, or mute the thread https://github.com/notifications/unsubscribe-auth/AQzA2TgYmzyUys1fOsM4qmQNApFznL97ks5rsJSAgaJpZM4Mvb2Y .

-- MIKLOS AUBERT BACKEND DEVELOPER EMAIL mat@trustpilot.com youremail@trustpilot.com PHONE +4531155565 DK Pilestraede 58, 5th Floor, 1112 Copenhagen K https://business.trustpilot.com/ http://www.facebook.com/trustpilot https://twitter.com/trustpilotdk https://www.linkedin.com/company/trustpilot https://plus.google.com/+TrustpilotReviews/posts https://www.youtube.com/user/trustpilot We're built on trust. Read about our values. https://www.trustpilot.com/trust EMAIL CONFIDENTIALITY NOTICE This email is from Trustpilot A/S registration No. DK30276582. The email and any attachments are confidential and may contain privileged information, and are intended for the named addressee(s) only. If you have received this message in error, please notify us and remove it from your system.

b-dur commented 7 years ago

OK. I'm actually a bit concerned if it is a good approach. Here is one scenario, where it would be unfortunate to have it. If a node.js Lambda function is using this module, and the SDK is writing console logs, then those will end up in Kibana. And if the message is multilines, then there will be duplicated logs with one line in each message. If there was a way to enable/disable the log functionality that would be great. Maybe the SDK could have a optional dependency to Winston, or a logger or log function could be passed via the configuration. What do you think?

martinbuberl commented 7 years ago

@b-dur @miklosaubert I removed the no-console override. It was copied from another project. It should only be allowed for actual console apps. Otherwise, if used and there's a need for it, use inline case by case // eslint-disable-line no-console or if used for logging - a wrapper should be used (as Bardur indicated). But this project is fine, so no need for it.