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

(chore) Clean up linting #2900

Merged
merged 3 commits into from
Dec 18, 2020
Merged

(chore) Clean up linting #2900

merged 3 commits into from
Dec 18, 2020

Conversation

Hirse
Copy link
Contributor

@Hirse Hirse commented Dec 2, 2020

Changes

  • Remove JSHint config
  • Switch ESLint config to JSON (for JSON-schema support)
  • Change npm lint script to include src/plugins folder
    • Fix some linting issues in the vue plugin
    • Fix some linting issues in the merge_html plugin
  • Add lint-languages npm script
  • Clean up lint workflow
    • Use latest node
    • Use npm ci for faster and more reliable dependency installs
    • Use new npm lint scripts
  • Add new ESLint configs for test and tools, which are not included in CI, but help during development

Checklist

  • Added markup tests, or they don't apply here because it's not a code change
  • Updated the changelog at CHANGES.md - NA?

.eslintrc.js Outdated Show resolved Hide resolved
@Hirse
Copy link
Contributor Author

Hirse commented Dec 13, 2020

@joshgoebel would you mind giving this another look?

@joshgoebel
Copy link
Member

What is the advantage of JSON again? It's very hard to review because JSON forces every single line to change when really nothing has changed.

@Hirse
Copy link
Contributor Author

Hirse commented Dec 16, 2020

What is the advantage of JSON again?

JSON can be validated with a schema. In some editors (I'm using VSCode) this is built-in and gives a great experience:
image

It's very hard to review because JSON forces every single line to change when really nothing has changed.

I don't quite understand what you mean here.

@joshgoebel
Copy link
Member

JSON can be validated with a schema.

Yes, that's true... though technically there is no reason it couldn't do the same exact thing based on the name of the file and using the default export... but whatever.

I don't quite understand what you mean here.

Instead of the diff showing what actually changed (a few lines) it just shows 200 lines removed, 200 lines added, etc... making it very hard to review. So I have to scroll thru the file manually comparing one line at a time visually... because I'm pretty sure at first glance you changed a few things and removed some of my comments, etc... And that is why I haven't found the time to sort this out yet. It's on the list to get to though.

Format workflow YAML to make it valid

Restore dev linting of languages

Use overrides instead of multiple files

Rebase on Master

Fix merge html plugin linting

Revert ESLint config to js
@Hirse
Copy link
Contributor Author

Hirse commented Dec 17, 2020

I've reverted the change to json (for this PR) and minimized the changes for easier comparison and reviewing.

.eslintrc.js Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Member

and minimized the changes for easier comparison and reviewing.

OMG, 100x better. Thank you.

@joshgoebel
Copy link
Member

@Hirse Thanks!

@joshgoebel joshgoebel merged commit 4d9d9e1 into highlightjs:master Dec 18, 2020
@Hirse Hirse deleted the linting branch December 18, 2020 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants