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

Added Linting Rule #1708

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

psankhe28
Copy link
Contributor

@psankhe28 psankhe28 commented May 3, 2024

Description

#1665

Changes

The no-console linting rule in ESLint discourages or prevents the use of console.log statements in your JavaScript codebase while allowing the use of other console methods like console.error, console.warn, console.info, and console.table.

Screenshots

I have tested it on a file called test.js and test1.js.
image
image

Output:
image

Tests

17/17 passed

@anth-volk
Copy link
Collaborator

Thanks for this contribution @psankhe28. Before I review, I just want to make sure - does this PR also remove garbage console.log statements and convert relevant ones to console.debug, console.error, etc.?

@anth-volk anth-volk self-requested a review May 6, 2024 12:53
@psankhe28
Copy link
Contributor Author

Thanks for this contribution @psankhe28. Before I review, I just want to make sure - does this PR also remove garbage console.log statements and convert relevant ones to console.debug, console.error, etc.?

It removes all console.log and allows console.debug, error etc.

@anth-volk
Copy link
Collaborator

anth-volk commented May 6, 2024

I'm not sure you understood my question. In at least 8 files (they're actually highlighted by GitHub in the "Files changed" tab), there are still console.log statements that will be errors if we merge this code. This issue asks contributors to remove garbage statements (e.g., on line 14 in src/layout/Screenshottable.jsx) and to transfer relevant ones (e.g., line 69 in src/api/parameters.js) to either console.error or console.debug. This is also why the GitHub Actions that run now fail - there's like 20 statements that fail this added linting rule.

@psankhe28
Copy link
Contributor Author

I'm not sure you understood my question. In at least 8 files (they're actually highlighted by GitHub in the "Files changed" tab), there are still console.log statements that will be errors if we merge this code. This issue asks contributors to remove garbage statements (e.g., on line 14 in src/layout/Screenshottable.jsx) and to transfer relevant ones (e.g., line 69 in src/api/parameters.js) to either console.error or console.debug. This is also why the GitHub Actions that run now fail - there's like 20 statements that fail this added linting rule.

I think so I misunderstood the requirement. I will look into it again.

@anth-volk
Copy link
Collaborator

That's alright. The approach you took was good - we are looking to do what you did - just to also modify those existing statements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: PR: Changes Requested
Development

Successfully merging this pull request may close these issues.

None yet

2 participants