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

GitHub actions and eslint config - warning only for now #3723

Merged
merged 2 commits into from
Jun 19, 2022

Conversation

rathboma
Copy link
Collaborator

@rathboma rathboma commented Apr 27, 2022

@olifolkerd here's a much simpler start to your eslint journey:

  1. Adds github actions workflows that do two things:
  • checks no one has changed files in dist, or committed a yarn.lock
  • runs eslint, and (right now) doesn't ever fail, just prints warnings
  1. Adds good eslint config, but also adds a plugin to turn all errors into warnings for the timebeing

Some things you could do next:

If you want to, you can run npx eslint --fix and it will auto-fix a number of issues (like # tabs for indentation), but you will end up with a big PR of changes, but that's easier to manage anyway.

You could also enable eslint errors for all PRs and have it only look at changed files using a plugin like this:
https://github.com/tj-actions/eslint-changed-files

That way you could have contributors slowly fixing up your codebase as you go :-D

@rathboma
Copy link
Collaborator Author

I made a PR on my fork to show you how it would work: beekeeper-studio#2

See how it marks my PR as failed because my PR has dist changes. Just what we want!

@rathboma
Copy link
Collaborator Author

A nice benefit of doing it this way is you still get eslint warnings in VSCode, so you can fix problems over time if you don't want to do big PRs:

image

@olifolkerd olifolkerd added the PR Awaiting Merge The PR is awaiting merge in next release version label May 1, 2022
@olifolkerd
Copy link
Owner

Hey @rathboma

That looks great, i will schedule this for inclusion in the next minor release.

Cheers

Oli :)

@olifolkerd olifolkerd changed the base branch from master to 5.3 June 19, 2022 11:14
Repository owner deleted a comment from lekoala Jun 19, 2022
@olifolkerd olifolkerd merged commit fbfe451 into olifolkerd:5.3 Jun 19, 2022
@olifolkerd
Copy link
Owner

@rathboma

Thanks for getting this all togeather, im gonna merge it into the 5.3 branch and will have a play with it all ready for the release in a few weeks.

Cheers

Oli :)

@rathboma
Copy link
Collaborator Author

Off the top of my head you need to specify the directory?

npx eslint src

For example

@olifolkerd
Copy link
Owner

olifolkerd commented Jun 19, 2022

Hey @rathboma

All good, i was being a muppet, had the terminal in the wrong directory lol.

Thanks for the PR without the fixes, that was exactly what i needed, it has been really helpful to go through the issues one by one. Helped me understand about how eslint works and helped me to customize the rules a little more.

I have pushed this to the 5.3 branch (which will be released in a few weeks), along with all the rule tweaks and all the fixes that eslint highlighted was needed. Ive also remove the warnings only setting so things are classed as errors now i understand the ruleset.

Am i correct in assuming that once these have been merged into master, any future PR's will then automatically have the tests run because of the lint-and-test workflow you have added?

Cheers

Oli :)

@rathboma
Copy link
Collaborator Author

Correct! Yeah it'll help folks solve those problems on their own

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Awaiting Merge The PR is awaiting merge in next release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants