onedesign / generator-one-base

A foundation for One Design Company projects on the web.
Other
1 stars 1 forks source link

Don't output ESLint warnings as in-browser notifications. #109

Closed cmalven closed 6 years ago

cmalven commented 6 years ago

For instance, variables named in snake case (e.g. my_variable) will currently throw an in-browser notification, even though it is just a "warning" in ESLint and not an "error." In some situations, avoiding non-camel-case variable names is impossible because a 3rd-party package or API may require it.

brianjhanson commented 6 years ago

I think the better way to handle this is to use the eslint comment configuration around the code that has to break the rule. That way we're using one-off configurations for one-off situations.


let some_variable = Thirdparty.thing // eslint-disable-line

or 

// eslint-disable-next-line
let single_variable_name = "some thing"

or

/* eslint camelcase: 0 */
function camel_case_required () {
   // some code 
}
/* eslint camelcase: "error" */
cmalven commented 6 years ago

I considered that, but this particular situation seems to be a frequent enough occurrence that I'm not comfortable muddying up the code with comments for something that we've explicitly set as a "warning" and not an error. It's rare that I have a sizable project where some JS lib doesn't use snake-casing for their parameters, so I'd hate to make this practice of using eslint-comments something people have to know about and know how to do to avoid seeing this notification every time they open their browser.

Anybody else have an opinion on this one way or another? @molwells @michaelramuta @MikeMcChillin @collinjoyce @mkornatz @bernsno

brianjhanson commented 6 years ago

I guess I'd also be in favor of making non-camelCase an error rather than a warning.

If we're not surfacing the warning to the dev outside of the console, it seems like the warnings could / would be easily ignored anyways (I know if I don't have something blocking me the chances of ignoring them is well over 50%).

cmalven commented 6 years ago

I guess I'd also be in favor of making non-camelCase an error rather than a warning.

I think that's another thing for the team to vote on. If the team was in favor of doing so, then I agree that we shouldn't be hiding this message for the in-browser notification. However, I also feel like this should not be an error for the same reason stated above: its a frequent enough occurrence, outside of our direct control, and I'm not eager to add ugly code comments when it comes up. I wish there was a 3rd option – to have ESlint automatically ignore non-camel-cased strings related to 3rd-party code – but that's not an option that I'm aware of.

brianjhanson commented 6 years ago

screen shot 2017-11-16 at 10 32 17 am

Source

I do think that's a different PR though. This one about not outputting warnings to the browser is cool by me.

I added https://github.com/onedesign/generator-one-base/issues/110 to cover the actual change.