Here is a start to the design doc, I'm not sure on the formatting/what it should have.

https://docs.google.com/document/d/18VHyxIfvts5LycKOwLU6-fDT0aRx30XEl1ErafdvD18/edit?usp=sharing

Thanks @Oishikatta! I left some feedback in the doc.

Howdy @Oishikatta Any progress on this?

@Oishikatta how is this going?

Deassigning @Oishikatta due to lack of response.

Also adding notes from @Oishikatta's doc to the issue thread, for easy reference in the future:

Javascript

Currently in use: eslint 3.18.0. This version is behind the current (4.x) by one major release, but uses the same rule format. Rules written for this version will not require changes if updated to 4.x in the future.

Plugins already exist to cover some of the issues: https://www.npmjs.com/package/eslint-plugin-angular -- but may require eslint 4.x. It is possible to pick out specific rules and use them with the 3.x version.

Suggestion: eslint appears suitably extendable with rules/plugins that it can meet the requirements. Enable built-in rules, retrieve some from open source packages, and write others.

Specific issues:

• Missing semicolons at end of lines: enable built-in rule “semi” https://eslint.org/docs/rules/semi
• Use of ‘==’ rather than ‘===’: enable built-in rule “eqeqeq”https://eslint.org/docs/rules/eqeqeq
• Spurious console.log() statements: enable built-in rule “no-console”https://eslint.org/docs/rules/no-console

Python

Currently in use: pylint. Supports adding AST and raw/stream based checkers.

Suggestion: Enable relevant built-in pylint checkers, and write others.

Specific issues:

• Invalid / incorrectly-formatted type info in docstrings: Pylint can check docstrings in Google style, but may be difficult to integrate with existing codebase and Oppia practices. Will need to test before determining if it is possible to use. https://pylint.readthedocs.io/en/latest/technical_reference/extensions.html#id3
• In docstrings, don't have a space after the initial """: We can check for empty lines after """ by enabling pylint.extensions.docstyle, but would need a custom checker for detecting just a space(s) after.

For both file types:
If checks are simple and fit with existing checks in the pre_commit_linter.py script (e.g. regexp), consider adding there first.

@seanlip can I go ahead and implement custom lint checks in pre_commit_linter or does this still need for looking online available options?

I think some of these can be solved with online available options, and some can be solved by adding custom lint checks. I'd recommend handling the former before the latter, since that might save work (e.g. adding a single line to the eslint config is better than writing a new function in the pre_commit_linter.py file).

Hello @seanlip! I am willing to work on this issue. Can you please tell me how should I go about this?
Thanks!

Hi @apb7! The instructions given above should be clear, so please follow them. Feel free to take just one of the checkboxes for your first PR.

In general, here's what to do:
(1) Pick an issue in the list above to tackle.
(2) Make that error in a file. Then run scripts/pre_commit_linter.py and see if it catches that error. If it does, great -- it means that error has somehow been fixed and we don't need to fix it anymore. If it doesn't, find a way to make the script catch it, so that scripts/pre_commit_linter.py fails with an error. (Look at the source code for that script to see what it does, and follow through on the references to various files like .eslintrc etc.)

In general I would advise picking an issue that's simple to solve for your first PR here -- it may be adding a new rule in .eslintrc or pylint, or doing some customization of an existing rule. But if you can't do that then you may need to write some kind of function that checks for these things manually. There are some examples of that in the linter script. The less code you write, the better, though.

Hello @seanlip! For checking alphabetized imports in Python, we can use flake-8-import-order. This package will add 4 new flake8 warnings.
Thanks!

@seanlip: For 'checking alphabetized imports' in Python, instead of flake-8-import-order we can use isort. To use it, we first need to install it using install_third_party.sh , create a .isort.cfg in the root directory and then implement the respective function in pre_commit_linter.py.
I have tested the above method and it works.

isort seems reasonable to me. Could you please submit a PR? Note that, by default, it shouldn't automatically mutate the code; it should just flag the errors when being run in the linter script. (Although it's fine if you want to use the auto-fix option to clean up existing code.)

Thanks!

@seanlip: isort gives both the options - auto-fix as well as a check for import order. We can set it according to our requirements.
I'll submit a PR soon!
Thanks!

oppia.directive(...)?

Yes, got it. Thanks!

Hi @seanlip, I have implemented the scope check using pyjsparser. This parser creates an AST tree for JS files using esprima settings. There are a number of directives which do not have a scope. What should be done for them? Also, some have scope set to true. What needs to be done for them as well?
Thanks!

Okay, understood. Thanks!

@seanlip: For directives not having any scope, I'm adding scope: false which is the default value of scope but I had a doubt. I think some directives having scope: true in extensions/objects/templates are not being used. For these, can we have scope: {}? Please correct me if I am going wrong somewhere.
Thanks!

By default all scopes should be scope: {}.

One thing to be careful about is that this might break scope inheritance, in which case we'll need to find some sort of workaround. In some cases that might be simple and in other cases that might be harder, so it may be worth sorting out the easy ones first and leaving the harder ones for further discussion or a separate PR.

@seanlip: If any directive does not have an explicitly defined scope, then it is equivalent to scope: false (Please see this link). Therefore we should use this instead of scope: {} for such directives. Please correct me if I am going wrong somewhere. Also, I am planning to make an initial PR for such directives along with the checker (not enabled though) for a review. How does that sound?
Thanks!

Actually, what I want to introduce is an explicit isolate scope for all directives, rather than allowing them to inherit parent scope. Hence the default of scope: {}.

Initial PR sounds good, thanks!

Hi @WickedBrat, I think @apb7 is working on it (As mentioned here #4592), if so then maybe you can take up another part of this issue else you can start working on this :).
(Let's wait for @apurv response)

@WickedBrat, I think apurv is working on "directives should have explicit isolated scope" so yeah you can work on "All directives should ave restrict: 'E'" (Also, you can take #4592, #4734 for reference)

Hi @WickedBrat! You can go ahead with it. I've picked up scope checks for now. Also, I suggest you go through the review comments in PR #4592 once. It would surely help!
Thanks!
(Thank you @DubeySandeep :) )

Hi @WickedBrat, please go through these comments in the above issue thread. Also, you don't need to write any check in pre-commit linter. You have to enable the directive-restrict rule in eslint-plugin-angular. Another important thing is to change the HTML attributes of the directives to elements in the HTML files where these directives are being used. This needs to be done without breaking any test (Also, test this manually as well). You can refer to PR #4592 and PR #4734 for the procedure. Thanks!

This issue has been broken down into smaller sub-issues for the remaining parts. Therefore closing this. Thanks!