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

Fix eslint errors, except no-multi-spaces #80

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

Conversation

nextgenthemes
Copy link

@link does not exist, I stumbled on some VSCODE issue (I use Atom) that said that it handled markdown so I just converted the link to MD

You may want to allow no-multi-spaces in eslint conf the WP coding standards for PHP actually require you do align spaces but in JS is disallowed, seems strange to me.

@link does not exist, I stumbled on some VSCODE issue (I use Atom) that said that it handled markdown so I just converted the link to MD

You may want to allow `no-multi-spaces` in eslint conf the WP coding standards for PHP actually require you do align spaces but in JS is disallowed, seems strange to me.
@justintadlock
Copy link
Member

I don't like these changes. We should configure eslint to ignore notices for these items instead.

@nextgenthemes
Copy link
Author

nextgenthemes commented Sep 3, 2019

Well I think the forced const/let make a lot of sense, forced brackets make sense to me as well. The @link seems not part of any spec or whatever. But if you do not like it then close it. I thought you were about sticking at least close to what the WP teams recommend since you are part of the Theme review team. But I do not get the no-multi-spaces at all and the padded-blocks I personally like and actually let it in here as well so I will for sure disable these two in my config.

@justintadlock
Copy link
Member

Well I think the forced const/let make a lot of sense,

In this particular case, it's pedantic and is an unnecessary change.

forced brackets make sense to me as well.

I prefer without the brackets since we're using the shorter-style function call. Just a personal style choice.

The @link seems not part of any spec or whatever.

Just aiming for consistency with the PHP files here. If there's an equivalent for JS, I'd be happy to use it.

I thought you were about sticking at least close to what the WP teams recommend since you are part of the Theme review team.

The TRT has no position on JS code formatting.

But, my personal view on code formatting in general is that it will follow "Justin Style". While that generally follows some standards similar to WP, I don't strictly follow any one style guide. Currently, how I write JS is a work in progress, coming from a primarily PHP background. So, this could evolve.

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