denoland / deno_lint

Blazing fast linter for JavaScript and TypeScript written in Rust
https://lint.deno.land/
MIT License
1.53k stars 172 forks source link

feat(rules): add no console log rule #1189

Closed 0xIchigo closed 1 year ago

0xIchigo commented 1 year ago

The goal of this PR is to address the this issue presented in the main Deno GitHub repository, which proposes the implementation of a no-console rule similar to that of eslint. In order to add such a rule to deno lint in the main Deno repository, we need a rule defined in the linter itself.

This PR achieves that goal by adding in a NoConsoleLog lint rule that checks whether a console.log() statement is in the specified code

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

0xIchigo commented 1 year ago

Damn, thank you for the suggested changes @lino-levan! My brain has been in Rust-mode for the last little while and I didn't even realize I put the code examples in Rust instead of TypeScript :tired_face:

The suggested changes have been implemented with this commit

0xIchigo commented 1 year ago

Thank you for the suggestions @marvinhagemeister! I agree with them and have fixed with this commit. LGTM 🔥

bartlomieju commented 1 year ago

I'm not convinced this should be in the recommended set - this means that any CLI tool that wants to output to console will spawn a plethora of errors. Maybe we should add it by default to the fresh set and other users can enable this rule as they see fit?

marvinhagemeister commented 1 year ago

@bartlomieju good point. Let's just add this rule without adding it to any preset. I don't want this to enable by default for fresh users but I'd like to opt-in to that for the fresh codebase itself.

0xIchigo commented 1 year ago

I agree @bartlomieju @marvinhagemeister - definitely think this rule should be off by default but allow users to opt-in, most likely for a fresh codebase. Is there anything else I'd need to add in order to make this an optional rule or is adding the functionality to the rules directory enough?

0xIchigo commented 1 year ago

Gentle bump @bartlomieju - is there anything else to add in order to make this an optional rule, not added to any preset by default, or is adding the functionality to the rules directory enough?

lev-blit commented 1 year ago

maybe this is for a future PR, but I find myself committing console.debug a lot as well (perhaps even more so than console.log)...

perhaps it would be useful to be able to customize the behavior of this like it's possible in eslint's no-console rule to either disallow all console functions or to specify which are allowed (error for example).

0xIchigo commented 1 year ago

@lev-blit yeah, that wouldn't be too hard to implement! I don't mind opening up another PR to tackle that issue once this one is merged. The nice part of how the code is structured is that in the implementation for NoConsoleLogHandler we have the following if check: if prop_ident.sym().as_ref() == "log" - you can simply swap out the "log" for "debug" in an entirely new rule, or modify the code to respond with different messages and errors based on whether you have a console.log() or a console.debug()