Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ESLint with minimal config #2831

Merged
merged 2 commits into from
Mar 26, 2021

Conversation

RunDevelopment
Copy link
Member

@RunDevelopment RunDevelopment commented Mar 25, 2021

Motivation:
#1796 seems to have become stale because of disagreement over indentation style and the sheer number of changes (look at the number of conflicts).

Description:
This PR adds ESLint with a minimal config. This means that the ESLint config only contains checks for possible errors and best practices. It does not contain style rules.

The idea is to get ESLint now and add style rules later. That way, we can benefit from error checking now and can discuss the style we want to enforce in detail in dedicated PRs. Each dedicated PR will only add/change one rule (or a small number of rules), so we can move quickly and review each rule change individually.

Future PRs:
There are a few PRs I will make as soon as this one got merged:

  1. A PR to re-enabling disabled rules with TODOs. A few rules are disabled because fixing them (while possible) isn't easy and because I wanted to keep the diff of this PR as small as possible.
  2. A PR to enforces our string quote style.
  3. A PR to enforce semicolons.

Changes

  • Added ESLint + config
  • Added GH actions CI job for linting
  • Fixed all linting errors (while trying to keep the diff as minimal as possible)

@github-actions
Copy link

github-actions bot commented Mar 25, 2021

JS File Size Changes (gzipped)

A total of 5 files have changed, with a combined diff of -4 B (-0.1%).

file master pull size diff % diff
components/prism-openqasm.min.js 563 B 562 B -1 B -0.2%
components/prism-parser.min.js 664 B 664 B 0 Bytes 0%
components/prism-robotframework.min.js 599 B 598 B -1 B -0.2%
plugins/jsonp-highlight/prism-jsonp-highlight.min.js 1.26 KB 1.25 KB -4 B -0.3%
plugins/treeview/prism-treeview.min.js 432 B 434 B +2 B +0.5%

Generated by 🚫 dangerJS against e5bc929

Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think that's a good strategy, add minimal ESLint at first, then add rules gradually.

@LeaVerou
Copy link
Member

Btw at some point we should rewrite all these var into let and const.

@RunDevelopment
Copy link
Member Author

Btw at some point we should rewrite all these var into let and const.

Guess I'll make a PR for the no-var as well after this :)

@joshgoebel
Copy link

Guess I'll make a PR for the no-var as well after this :)

Welcome to the future, Prism.js! Glad to see this happening.

@LeaVerou
Copy link
Member

Btw at some point we should rewrite all these var into let and const.

Guess I'll make a PR for the no-var as well after this :)

I am envious of your energy! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants