-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
🏗♻️:rm js console expressions as prep for logging framework adoption #1157
Conversation
It is considered a best practice to avoid the use of any `console` methods in JavaScript code that will run on the browser. **NOTE:** If your repository contains a server side project, you can add `"nodejs"` to the `environment` property of analyzer meta in `.deepsource.toml`. This will prevent this issue from getting raised. Documentation for the analyzer meta can be found [here](https://docs.deepsource.com/docs/analyzers-javascript#meta). Alternatively, you can silence this issue for your repository [as shown here](https://deepsource.com/blog/releases-issue-actions). If a specific `console` call is meant to stay for other reasons, you can add [a skipcq comment](https://docs.deepsource.com/docs/issues-ignore-rules#silencing-a-specific-issue) to that line. This will inform other developers about the reason behind the log's presence, and prevent DeepSource from flagging it.
👷 Deploy Preview for open-inf-is processing.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thnx. but solution should not be having no replacement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c5427e7
to
8c69b01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even as ex-@nodejs core, my position on this PR is still -1 BUT
and a big BUT: a logging framework should preferably be used
merging in good faith effort & also propose our switching @pinojs
See: https://getpino.io
✅ Deploy Preview for openinfis ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for openinfis ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Derek Lewis <dereknongeneric@open.inf.is>
See OpenINF/nps — fork of project repo @ OpenINF-private/openinf-nps (with no known issues to date). |
this dang merge conflict is w/e — we can let this one rot here, but we can address insufficiencies we have found w/ off-the-shelf nps using @pinojs is necessary |
PR URL: #1157 Reviewed-by: Derek Lewis <dereknongeneric@open.inf.is> Reviewed-by: OpenINF-bot <openinfbot@open.inf.is> Signed-off-by: Derek Lewis <dereknongeneric@open.inf.is> Cc: Grimes <grimes@open.inf.is> Refs: https://github.com/OpenINF/nps Co-Authored-By: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com> Co-Authored-By: Derek Lewis <dereknongeneric@open.inf.is>
It is considered a best practice to avoid the use of any
console
methods in JavaScript code that will run on the browser.NOTE: If your repository contains a server side project, you can add
"nodejs"
to theenvironment
property of analyzer meta in.deepsource.toml
.This will prevent this issue from getting raised.
Documentation for the analyzer meta can be found here.
Alternatively, you can silence this issue for your repository as shown here.
If a specific
console
call is meant to stay for other reasons, you can add a skipcq comment to that line.This will inform other developers about the reason behind the log's presence, and prevent DeepSource from flagging it.