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

Allow console.warn and console.error #5

Merged
merged 1 commit into from
Mar 7, 2017
Merged

Conversation

limonte
Copy link

@limonte limonte commented Mar 6, 2017

In order to implement vaadin/vaadin-grid#566 we need to allow console.warn. In future we will also need console.error.

console.log will still throw a linter error.


This change is Reviewable

@Saulis
Copy link

Saulis commented Mar 6, 2017

I'm thinking if we should keep the default rules and add explicit ignores to the code using console as it should be quite rare?

Also, I wonder how this passes the linter: https://github.com/vaadin/vaadin-grid/blob/2.0-dev/vaadin-grid-dynamic-columns-behavior.html#L114

@limonte
Copy link
Author

limonte commented Mar 6, 2017

I'm thinking if we should keep the default rules and add explicit ignores to the code using console

there are no "default" rules, no-console is the recommended rule, allowing .warn() and .error() is documented: http://eslint.org/docs/rules/no-console#options

it should be quite rare

Actually, we should use console.warn() more often to improve the DX.

Also, I wonder how this passes the linter: https://github.com/vaadin/vaadin-grid/blob/2.0-dev/vaadin-grid-dynamic-columns-behavior.html#L114

Probably, it's because of the workaround with the window. context.

@tomivirkki
Copy link
Member

I'm also in favor of keeping the rule since console.log is used a lot in development and while the rule is there, they don't accidentally end up in PRs.

I used window.console.warn to work around the linter and I think it's a neat way to explicitly mark console messages that are intended to be merged (explicit ignores are also ok IMO).

@limonte
Copy link
Author

limonte commented Mar 7, 2017

I'm also in favor of keeping the rule since console.log is used a lot in development and while the rule is there, they don't accidentally end up in PRs.

console.* including console.log will produce a linter error, exceptions are console.warn and console.error.

@tomivirkki
Copy link
Member

console.* including console.log will produce a linter error, exceptions are console.warn and console.error.

Ah, right. Well in that case LGTM

@limonte limonte self-assigned this Mar 7, 2017
@Saulis
Copy link

Saulis commented Mar 7, 2017

:lgtm:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@Saulis Saulis merged commit 6fbf5b8 into master Mar 7, 2017
@Saulis Saulis deleted the console-warn-error branch March 7, 2017 08:15
@Saulis Saulis removed the in review label Mar 7, 2017
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