Project

General

Profile

action #91638

Ensure standard javascript code

Added by cdywan 3 months ago. Updated 1 day ago.

Status:
Resolved
Priority:
Low
Assignee:
Category:
Feature requests
Target version:
Start date:
2021-04-23
Due date:
% Done:

0%

Estimated time:
Difficulty:

Description

Motivation

Our currenty Javascript linter js-beautify lives up to its name, the code looks a bit nicer but it doesn't cover much. It'd be desirable to flag more logical aspects like uses of const and not just aesthestics of the code.

Acceptance criteria

  • AC1: semistandard and more common javascript code styles are ensured

Suggestions

History

#1 Updated by okurz 3 months ago

  • Subject changed from Consider Standard or to Ensure standard javascript code
  • Description updated (diff)
  • Status changed from New to Workable
  • Priority changed from Normal to Low
  • Target version changed from future to Ready

Looks reasonable. I suggest to start from https://github.com/mojolicious/server-starter/blob/master/.github/workflows/test.yml and apply what we need in openQA as well.

#2 Updated by kraih 3 months ago

I wouldn't mind taking care of this. Unless someone else would like to use the opportunity to learn more about JavaScript dev tools. My recommendation would be:

  1. Put a package.json and eslint.json into the repo that contain all the deps and settings (semi-standard + extended max line length)
  2. Create a tools/eslint script that will check if node is installed (give a useful message otherwise), update deps with npm in the local node_modules, and then run eslint --fix over our JavaScript files (make sure it complains about violations it can't fix)
  3. Add a GitHub Action that runs eslint over our JavaScript files and annotates pull requests with rule violations
  4. Make sure all the temporary npm files are added to .gitignore
  5. Update relevant documentation

Doing it this way also means that clever editors such as VSCode can use the settings to lint your code as you write it.

#3 Updated by cdywan 3 months ago

Just one thing comes to mind that you didn't mention. Make sure that we have a makefile target that does the linting and which is used in our CI - I don't care that much about installing npm and such here, but it should be trivial to reproduce whatever we do in CI without looking up command lines to execute. This has been a source of problems before. From a glance the mentioned GHA runs 3 different commands.

#4 Updated by kraih 3 months ago

cdywan wrote:

Just one thing comes to mind that you didn't mention. Make sure that we have a makefile target that does the linting and which is used in our CI - I don't care that much about installing npm and such here, but it should be trivial to reproduce whatever we do in CI without looking up command lines to execute. This has been a source of problems before. From a glance the mentioned GHA runs 3 different commands.

Actually i think a separate GitHub Action would be better, since those can annotate the pull request diff with eslint rule violations.

#5 Updated by kraih 3 months ago

  • Assignee set to kraih

I'll make a proof of concept to show GitHub PR annotations.

#6 Updated by kraih 2 months ago

I've prepared a very large draft PR that replaces jsbeautifier with eslint and semistandard rules + GitHub Action. https://github.com/os-autoinst/openQA/pull/3900

#7 Updated by kraih 2 months ago

  • Assignee deleted (kraih)

#8 Updated by kraih 2 months ago

The PR also demonstrates how eslint is about more than just formatting, it's a proper linter that can find many other issues for us.

#9 Updated by kraih about 2 months ago

I think one thing the eslint results have shown is that our JavaScript code is in a very poor state. Merely running a prettifier over it is not gonna solve that, we also need to make a decision how we want to organise it (ESM etc.).

#10 Updated by okurz about 1 month ago

  • Target version changed from Ready to future

#11 Updated by kraih 11 days ago

Here's another idea, ESLint with Prettier formatting rules. That has the advantage that we can start only with formatting rules and later add more. https://github.com/os-autoinst/openQA/pull/4042

#12 Updated by kraih 8 days ago

  • Status changed from Workable to In Progress
  • Assignee set to kraih

#13 Updated by kraih 8 days ago

  • Status changed from In Progress to Feedback

The PR has actually been merged, so we now have ESLint (with only one rule activated) and the Prettier formatting standard. Rule violations will result in PR annotations on GitHub, and ESLint extensions can be used with IDEs. That should resolve this ticket. Next steps will be to enable more strict rules in ESLint and to fix our JavaScript code.

#14 Updated by kraih 8 days ago

This commit shows one ESLint rule being enabled as an error. More can follow the same way, or be enabled with a category such as eslint:recommended.

#15 Updated by okurz 8 days ago

kraih wrote:

[…] That should resolve this ticket.

ok, but why did you not "resolve" it then? Do you think we need more rules enforced?

#16 Updated by kraih 2 days ago

  • Status changed from Feedback to Resolved

okurz wrote:

kraih wrote:

[…] That should resolve this ticket.

ok, but why did you not "resolve" it then? Do you think we need more rules enforced?

Fair point, if i leave this unresolved it could stay open forever since there will always be new ESLint rules for us to enable in the future.

#17 Updated by okurz 2 days ago

kraih wrote:

Fair point, if i leave this unresolved it could stay open forever since there will always be new ESLint rules for us to enable in the future.

makes sense, thanks. We can update/add rules over time.

#18 Updated by tinita 1 day ago

  • Target version changed from future to Ready

Also available in: Atom PDF