[GitHub] tedmiston commented on issue #3656: [AIRFLOW-2803] Fix all ESLint issues

[GitBox <gi...@apache.org>]

tedmiston commented on issue #3656: [AIRFLOW-2803] Fix all ESLint issues
URL: https://github.com/apache/incubator-airflow/pull/3656#issuecomment-415647080
 
 
   @verdan Please see the revised diff / new squashed commit, and review when you have a chance.
   
   There were some opinionated decisions to make here as far as which warnings / errors to fix via refactoring vs which were more complicated and worthy of disabling via a linter comment... there's a decent amount of tech debt in the front end JS.  I started off doing more refactoring, but eventually saw the size of this diff growing and growing and switched to only refactoring what was absolutely required for right now.
   
   Some notes:
   
   - This diff overall now resolves something like ~1500–2500 linter errors in total.  In some places I have used disable linter calls if refactoring the code to pass felt cumbersome or like a bit much.  I think these few long tail errors could be cleaned up with refactoring post-PR.
   - I have an example of refactoring building the long query string with template strings here - https://github.com/apache/incubator-airflow/pull/3656/commits/b65388a7b772fa7b40140d77ef25c45ad1c475c9.  Since the PR grew pretty huge already, I'm thinking of maybe pulling it out of this PR and refactoring the others requests separately if this style pattern feels like an improvement.  Rewriting this also made me realize we have a dormant bug where some query params aren't being encoded which is extra motivation to migrate this part into a separate PR and keep scope here focused on linting.
   - The original [eslint-plugin-jinja](https://github.com/alexkuz/eslint-plugin-jinja) is not maintained and [doesn't](https://github.com/alexkuz/eslint-plugin-jinja/issues/3) [work](https://github.com/alexkuz/eslint-plugin-jinja/issues/4) with current version of ESLint, however, the [jupyterlab/eslint-plugin-jinja](https://github.com/jupyterlab/eslint-plugin-jinja) fork does, so I went with that one.
   - Apparently there's an issue with `eslint --fix` when using the eslint-plugin-jinja plugin / some plugins where it just doesn't work but also doesn't given an error.  "Unfortunately, auto-fix is not supported on plugins with processors (i.e., plugins which transform files to be ready for linting in JS)." (https://github.com/eslint/eslint/issues/7456).
   - My workaround for this was to temporarily comment out the plugin to run `--fix` which will fix some things but also break some things (which the plugin would prevent) inside Jinja, but since it can auto fix many errors, it was still helpful.  Then when done, uncomment the plugin, fix any auto errors by hand, and re-run lint.  [I'm considering adding this to our docs since it was unexpected.]
   - I have 2 FIXMEs outstanding - 1 in graph.html, 1 in tree.html where the linter is convinced the lines should not end with a semicolon (check the error).  I haven't figured out how to solve these yet if you have  any advice.
   - Aligned all outermost vertical indentation to match level of script tags.
   - Ensured consistent indentation for script tags nested inside Jinja blocks.
   - Ensured newlines surrounding every function definition.
   - Added blank newline after Apache license text where missing for consistency.
   - Added missing closing </div> tag in circles.html.
   - Needs tested from the GUI.
   - Also just realized I forgot to revert the use of ES6 syntax for anonymous functions, so I'll have to fix that.  Otherwise, I'm hoping this is very close to done.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services