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

add no-print rule to enforce use of logger #19

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

slheavner
Copy link
Contributor

Similar to the no-console rules for eslint/tslint. Enforces the use of a logging framework rather than raw print statements.

@@ -56,6 +56,11 @@ export default class CodeStyle {
);
}
}
},
PrintStatement: s => {
if (noPrint) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If not off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I made no-print rules a RulesSeverity and I actually meant to remove this if.
'no-print'?: RuleSeverity;
If you'd rather it be a different type lmk

noPrint: (range: Range, severity: DiagnosticSeverity) => ({
severity: severity,
code: CodeStyleError.NoPrint,
message: `${CS} don't use direct Print statements`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're doing another round of review, maybe:
"Avoid using direct Print statements"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds proper

@elsassph
Copy link
Collaborator

@slheavner I think you should remove the package-lock.json from your PR. This upgrades forces others to use npm 7 apparently.

@slheavner
Copy link
Contributor Author

was wondering why that updated, reverted.

@elsassph elsassph merged commit 13157bd into rokucommunity:master Apr 16, 2021
@elsassph
Copy link
Collaborator

Thanks!

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.

2 participants