Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyzer): noConsoleLog rule #4357

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Apr 7, 2023

Summary

Implements the rule noConsoleLog.

Closes #4223

This rule prevents the usage of console.log, while all the other functions from console are allowed.

Test Plan

Added tests

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Apr 7, 2023

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 3690a31
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/642fe7d47792770008e71c29
😎 Deploy Preview https://deploy-preview-4357--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Project Area: project configuration and loading labels Apr 7, 2023
@netlify
Copy link

netlify bot commented Apr 7, 2023

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 3f2fc0c
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/642ff153469074000898f43b
😎 Deploy Preview https://deploy-preview-4357--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

It could be nice to add other valid codes such as:

console.info("info")
console.warn("warn")
console.error("error")
console.assert(true)

@ematipico
Copy link
Contributor Author

ematipico commented Apr 7, 2023

It could be nice to add other valid codes such as:

console.info("info")
console.warn("warn")
console.error("error")
console.assert(true)

Where precisely? In the test suite or the documentation?

EDIT: I have done both

@ematipico ematipico requested a review from Conaclos April 7, 2023 10:32
@ematipico ematipico merged commit 6cb9585 into main Apr 7, 2023
@ematipico ematipico deleted the feat/no-console branch April 7, 2023 12:44
@jpike88
Copy link

jpike88 commented Apr 7, 2023

Yesssss you guys are absolute machines

@jfmengels
Copy link

I'm curious (but I don't need it): Is there a reason that things like globalThis.console.log() are not reported in this implementation?

@ematipico
Copy link
Contributor Author

Uncaught case from my side :)

Although all the rules don't take that into consideration, probably because most of the users (myself in primis) don't use it, so it was missed.

@jfmengels
Copy link

I don't use it either, but it's an easy workaround to the rule.
I'm actually surprised that some of the rules do, nice 👍
ESLint doesn't seem to be handling this, I guess partially because globalThis is somewhat new?
(Also, just in case you will be tackling it, apparently globalThis.globalThis.globalThis.console.log(1) is possible 🙃 )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Project Area: project configuration and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 noConsoleLog
4 participants