Add a linter

[Treora]

To automate code quality checking.

• [x] Choose a linting tool (JSHint, ESLint, ...?).
• [x] Configure it to what seems a good coding style; preferrably endorsing some existing standard.
• [ ] Run as a check in make test (yet to be created).
• [x] Run and print warnings in make build.
• [x] Run and print warnings in make watch.

[obsidianart]

I would try to have a linter which autofix things (like tabs and spaces) when you commit

[reficul31]

I would like to take up this issue as I have already made some headway after having a brief chat with @Treora . According to me the steps to follow are.

1. Observe and document the coding style
2. Make a ESLint config file with the coding styles observed
3. Bootstrap the linter and run tests for all the packages already made.

I intend to do this as an experiment to get to know the coding style a bit better and so that it also helps in the CI we are trying to set up.

[reficul31]

Following my experiment I have tackled some problems. As per the suggestion by @Treora I found a standard we should use for the project, but right now the code is not at all consistent. As I see it I can go two ways. Either fix the code to suite the style or fix our own custom standard which again has some problems as the code isn't consistent throughout. Any feedback will be extremely helpful. To follow the progress

[reficul31]

So I have made some significant progress. I have already added the eslint extending the standard config provided by default. I have also integrated the eslint-react plugin to allow jsx to be parsed. To see the progress. If I could get the go signal to refactor the styling I will start off by changing the style of the src module first. Thanks.

[Treora]

@reficul31: Good progress indeed! I would not mind changing some code style by the way, e.g. using two-space indentation is fine with me if those standards all do that too.

I just did quite a refactor to use the async/await syntax throughout the code. Shall we first merge that and then fix the styles?

[reficul31]

@Treora I just now added a bit of cleaned code. Please review it once you are free and see if the style suits the project.

I would not mind changing some code style by the way, e.g. using two-space indentation is fine with me if those standards all do that too.

It has been changed in the most recent commit. Also it doesn't matter as soon as we put an editor config in place the indentation error will be sorted out itself.

I just did quite a refactor to use the async/await syntax throughout the code. Shall we first merge that and then fix the styles?

Sorry, I didn't look at the comment until it was too late. I will rebase the branch and push again. Please tell me if any changes in the styling is required.

[obsidianart]

@reficul31 are you manually fixing those file or using autolint? http://eslint.org/docs/user-guide/command-line-interface --fix #Automatically fix problems I would be nice to add the autolint in the pre-commit hook so we don't have to worry about writing linted code.

Also, please be extra carefull about naming the commit for the linter correctly so we can use hyper-blame (https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html) to remove them when checking who wrote a line. If you forget to do so it will be much harder to understand who wrote what and why.

[reficul31]

are you manually fixing those file or using autolint?

@obsidianart Yes, the CLI tool for the eslint is being used to fix trivial errors such as spaces and curl braces. Although for more serious warnings and errors manual tweaking will need to be done. Although the plan for later is integrating the standard js lib to allow more CLI tools as suggested by @sanjo .

I would be nice to add the autolint in the pre-commit hook so we don't have to worry about writing linted code.

I will look into it and try and try and integrate it. Although I highly suggest running the linter once on the local machine to remove errors that cannot be removed by the --fix.

Also, please be extra carefull about naming the commit for the linter correctly so we can use hyper-blame (https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html) to remove them when checking who wrote a line. If you forget to do so it will be much harder to understand who wrote what and why.

I had no idea about this, I will read up and get back to you.

edit: I have also added the editor config file. It contains some of the standard rules for a project. Please tell me if any modification is required. Progress

[obsidianart]

@reficul31 the pre-commit hook do run on the local machine

[Treora]

@reficul31: I just pushed the conversion to async/await syntax (#81), apparently I forgot pushing that yesterday. We should rebase one of these two style changes onto the other, either way around.

As for style itself, most of it looks like an improvement. A few questions already:

• you specified dangling commas to be required/acceptable, but they are still removed from the code?
• you add es-plugin-promise, but do not specify rules or use their recommended set; does it do anything then?
• which rule or standard thinks && should go on the end of a line, rather than before the next line? I don't see it in standardjs's rules.

Perhaps make a PR when you're ready and we can go through it together.

[reficul31]

@obsidianart

the pre-commit hook do run on the local machine

Yeah I figured that out a bit later after I read through some docs. @Treora

We should rebase one of these two style changes onto the other, either way around.

I will try and rebase the branch, if I have some problems I will seek help.

you specified dangling commas to be required/acceptable, but they are still removed from the code?

I don't remember specifying dangling commas as acceptable. But we can if you think it hinders with your style of coding. This specific line of code means that it will give an error on every comma dangle except when a dangling comma is found on an object.

"comma-dangle": ["error", {
            "arrays": "never",
            "objects": "ignore",
            "imports": "never",
            "exports": "never",
            "functions": "never"
    }]

which rule or standard thinks && should go on the end of a line, rather than before the next line? I don't see it in standardjs's rules.

operator-linebreak

you add es-plugin-promise, but do not specify rules or use their recommended set; does it do anything then?

Seems, I forgot to add the recommended settings of the promise plugin. Before I send a PR there are some changes that will be needed to be done to the code to make it completely clean. I don't think I am qualified enough to make these style decisions. Therefore if someone could pull the branch and do the changes and push, it would be of immense help.

[Treora]

As for dangling commas, I would rather require than forbid them; setting comma-dangle to always-multiline seems good. As for operators and linebreaks, setting operator-linebreak to before would have my preference.

Let know which things you would like help on.

[reficul31]

@Treora

As for dangling commas, I would rather require than forbid them; setting comma-dangle to always-multiline seems good. As for operators and linebreaks, setting operator-linebreak to before would have my preference.

Both the configurations were added successfully. Although it messed up my commit log.

Let know which things you would like help on.

Could you please pull the branch and try the lint on your machine. It will give you 20 errors that I think a maintainer should only resolve as they are not design error rather they are logical errors. There are also some warnings. You could remove them from the config file all together, but I won't recommend it. Please tell me if any rule changes are required. I will redo them. Also I will rebase the async commit shortly after. Or should I just send a PR?

[ghost]

Can you please open a pull request for this.

I suggest to go with the standard code style without modifications to save time as argued in the standard readme.

[niroshn]

I added eslint and their configuration.

When running npm run eslint

It shows 54 errors and 238 warnnings

[niroshn]

I added configuration for react and ES6 . I think we need to refactor code for misspell thing

[reficul31]

I am sorry @niroshannrsh . This issue wasn't integration ready. If you would like to contribute to this issue link. This issue is still under development.

[niroshn]

@reficul31 I Know . I contributed to this issue. I added eslint.

[reficul31]

Can you please open a pull request for this.

@sanjo I am actually waiting for the async-await PR to be merged so that I can rebase the code base and then send the PR.

[reficul31]

As suggested by @Sanjo I have added standard, on a new branch though. The linter hasn't been run to remove the errors. Please check the configuration once so that I can clean the code. Link @Treora @Sanjo @obsidianart I have provided the option for both the setups, one with eslint and one with standard-own. Whichever one seems a good fit for the project can be used. Also once the async await PR is merged I will also need to add the async format rules also. @obsidianart I will add the git hook as soon as the initial configuration is done and the code is rebased.

[niroshn]

@reficul31 Did you see my eslint config file .

Is there any changes ?

[Treora]

I am actually waiting for the async-await PR to be merged so that I can rebase the code base and then send the PR.

I just merged it, sorry for the wait. You can also rebase onto branches before they are merged by the way, though it can become a bit messy if the branch is changed.

[Treora]

@reficul31: Feel free to create a PR when you want me to look at it and go through the remaining warnings and questions.

[reficul31]

@Treora @Sanjo Could you possibly look at both the configurations and tell me which one shall be used? The two methods are on different branches.

• Eslint Uses eslint configuration files and eslint default eslint cli to clean the code and lint.

• Standard Uses standard-own cli to clean the code and lint.

I will clean and rebase both the branches irrespective but I would require 1 to make the PR. Thanks.

Edit: Rebased both the branches. NOTE : The Standard branch doesn't contain the clean code yet. Once the configuration to be used is final, I'll be cleaning the code and adding the git hook. Important : It has recently come to my notice that the standard own configuration doesn't allow for warnings. Every warning is shown as an error.

[Treora]

Nice. I'll have a look at them now then.

[obsidianart]

If missing semicolon is a warning I wonder if the linter is configured correctly, it shouldn't be anything considering the style I've seen in the project.

[reficul31]

@obsidianart Sorry, but I don't see the missing semicolon warning. Would be specific to which rule you are talking about.

[obsidianart]

@reficul31 it's in the image uploaded...

[reficul31]

@obsidianart As I have no idea on which file the eslint is operating on in the screenshot, I will refrain from commenting. But if you refer to my PR. There are no such errors.

[niroshn]

I have no idea on which file the eslint is operating on in the screenshot, @reficul31 Add your eslint resopse massege screenshot here.

[reficul31]

@Treora For the task

Run and print warnings in make watch

Might I suggest something like eslint-watch. I have already integrated it with the project but it doesn't run with the make watch command as the npm style && requires one task to finish and then start on the other one. Since the watch command never ends the eslint watch never starts.We could use concurrently or should we go the simple way and just cascade the commands with a &? To see my Progress

[Treora]

Might I suggest something like eslint-watch https://www.npmjs.com/package/eslint-watch. Sounds good!

... it doesn't run with the make watch command as the npm style |&&| requires one task to finish and then start on the other one.

We could configure these things in the gulp. Perhaps by making the watch task depend on both a build-watch (= the current watch task) and a lint-watch.

I would not mind at all if scripts in package.json are just running gulp tasks of the same name (and the Makefile mostly just runs those npm scripts).

[Treora]

This has been fixed with #83 and #87, apparently forgot to close this issue.