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

lint: disallow console.log and similar #5087

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hayemaxi
Copy link
Contributor

  • Add lint rule to detect logging with console
  • Fix cases where this occurs
  • Add exception for cases where required, e.g.
    • logger.ts
    • any front end code
    • any language servers
    • tests
    • build scripts
  • Refactor logger to take only a dependency on vscode and nothing else. This is done by moving perfLogger to its own file.
    • This also adds a dependency to globals to have logger

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@hayemaxi hayemaxi requested a review from a team as a code owner May 31, 2024 15:50
@@ -3,8 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

// Disable because this is the main logger file.
/* eslint-disable aws-toolkits/no-console-log */
Copy link
Contributor

Choose a reason for hiding this comment

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

can we put this in the ConsoleLogger scope? shouldn't be needed for the rest of this file.

@@ -686,6 +686,8 @@ class ProcessTerminal implements vscode.Pseudoterminal {
public constructor(private readonly process: ChildProcess) {
// Used in integration tests
if (isAutomation()) {
// Disable because it is a test.
// eslint-disable-next-line aws-toolkits/no-console-log
this.onDidWrite(text => console.log(text.trim()))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for specific, intentionally cases. But it's good to have the rule generally enabled for tests.

@@ -187,6 +187,8 @@ export class WebviewClientFactory {
},
get: (_, prop) => {
if (typeof prop !== 'string') {
// Disable because we cannot log to extension in a webview.
// eslint-disable-next-line aws-toolkits/no-console-log
console.warn(`Tried to index webview client with non-string property: ${String(prop)}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we forward logs to the backend? Do we have a webview base class that could have a log() function?

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

Successfully merging this pull request may close these issues.

None yet

3 participants