littleball-games / lb-f

A collection of functional programming functions
MIT License
0 stars 1 forks source link

Replace standard module with eslint + standard preset #15

Closed skylize closed 6 years ago

skylize commented 6 years ago

This allows live linting in text editor against the JS Standard Style and still does linting in pretest script. Additional benefit is we can now add inline eslint overrides in special cases where the Standard style is causing harm by being too strict.

skylize commented 6 years ago

I think it is worth noting here that the standard module uses eslint under the hood. This change makes the package.json look more complex. But it actually reduces our real dependencies slightly while providing the correct config for in-editor live linting and gives us more flexibility going forward.

skylize commented 6 years ago

@Eruant Any issues with this patch?

Eruant commented 6 years ago

@skylize I need to test this out in my editor to see if there are any issues. I currently use neovim with syntastic which does this for me already.

...and gives us more flexibility going forward.

The idea of standard is to keep the linting to a standard format, so I don't think we need extra flexibility.

skylize commented 6 years ago

@Eruant

I need to test this out in my editor to see if there are any issues.

Ok. Please give it a try and get back to me. Thanks

I currently use neovim with syntastic which does this for me already.

Haven't tried neovim. I use Atom. It is possible for me to add a global installation of eslint and a global eslintrc to be used by Atom for linting. But there is not any way for me to match the linter in atom to the project's enforced style based on an installation of the standard module.

I do not use JS Standard Style in my own projects because it has a number of rules I disagree with. So installing it globally would be just plain bad for me. I need the project to carry the correct information to enforce the rules it expects.

The standard recommendation from the developers of ESLint is to install a local copy of eslint for each project and use project config files rather than global config files. I strongly agree with this recommendation.

The idea of standard is to keep the linting to a standard format, so I don't think we need extra flexibility.

The idea is to have some standard, and JS Standard Style gives a reasonable starting point for a standard. But it also includes some rules that are controversial and skips some rules we might find helpful. If we use the standard module, then we are 100% locked into whatever rules Standard tells us we have to use. If instead we use the eslint preset, then we still get all the benefits of starting from Standard style, but we can also potentially extend it with our own ruleset.

We are not forced to override anything. We can continue to write code against the Standard style, but if we conclude that a particular rule is getting in the way more than helping, or if we decide that an important rule is missing, then all it takes is one line in the eslint config to modify our project's standard for the changed rule.

But I only pointed that out as a side-benefit of this patch. The real point of this patch is so that I can and other developers can use live linting with the style that you have chosen to be the current standard without needing to do intrusive global installations.

Eruant commented 6 years ago

I've tested this out and it does not cause any issues my end.

I do not use JS Standard Style in my own projects because it has a number of rules I disagree with.

@skylize I would be curious to know what rules in standard you disagree with. Personally I've found that when there is a rule I initially don't like - it teaches me something new about my code and I learn from it. My code has improved since using it, and so I'm yet to disagree with it.

skylize commented 6 years ago

@Eruant My complaints are mostly stylistic.

Off the top of my head, I don't like the chosen brace style. On the occasion that I do an if/else with blocks, I like else to go on a new line instead of with the previous brace because that makes it a lot easier to read the else condition and gives nicer spacing around the separate blocks. I write without braces whenever possible. I think the braces around single expressions are just extra noise that distract from the real code.

Another one that comes to mind without looking anything up is the no-unused-vars rule. I understand the value in this rule, but it prevents things like const fnWithIgnoredParam = _ => 'some value', where I explicitly indicate the parameter is expected but ignored instead of implying there is no parameter expected.

And a lot of the enforced spacing rules go against the spacing I use..

I would not really expect you to change these rules, but I definitely need live linting to tell me about them. But there is no way you will get me to turn on these rules globally for my whole system.

There are also rules that I generally agree with, but have small potential for getting in the way. That's where inline overrides come into play. You can put a comment in the code that tells eslint to ignore the rule, and anyone reading the code knows you broke the rule on purpose and can interpret accordingly. A couple examples are no-constant-condition and no-sparse-arrays (not great examples, but examples nonetheless). You aren't likely to need to override these, but it's nice know we can.

I also like to use inline overrides for temporary things while I'm working, such as (when no-unused-vars is turned on) disabling no-unused-vars for a block that I'm writing from scratch until there is enough logic in place for that rule to have meaning again.

We could also add things that make the project standard even more strict than JS Standard. It seems like you are on board with using const for declaration. There is a prefer-const rule that Standard omits. We could add that rule and it would yell at people trying to use let or var unless they reassign the variable's value later in the code.

Eruant commented 6 years ago

I'm not sure where you are coming from for all of your arguments:

Another one that comes to mind without looking anything up is the no-unused-vars rule. I understand the value in this rule, but it prevents things like const fnWithIgnoredParam = _ => 'some value', where I explicitly indicate the parameter is expected but ignored instead of implying there is no parameter expected.

This does not throw any errors using standard

I also like to use inline overrides for temporary things while I'm working, such as (when no-unused-vars is turned on) disabling no-unused-vars for a block that I'm writing from scratch until there is enough logic in place for that rule to have meaning again.

You can add eslint hints without needing to extend standard.

I'm going to accept the PR as it will enable you to have code hints in your editor. I don't think we should extend standard in the future (the whole point of standard is to remove the hastle of code style)

skylize commented 6 years ago

I'm going to accept the PR as it will enable you to have code hints in your editor. I don't think we should extend standard in the future (the whole point of standard is to remove the hastle of code style)

Thanks. 👍

That's fine with me. As I've said several times already, the point of this was not to extend Standard, but rather to make it easier to follow best practice of letting editors pull the linter and linting rules from the project instead of from a global.

I just thought potential to extend rules sometime in the future (if we run into a worthwhile reason to) was a nice bonus worth mentioning. I only even discussed particular rules because you asked.