-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add linting step within GitHub Actions workflow #196
Conversation
.github/workflows/main.yml
Outdated
@@ -51,3 +51,10 @@ jobs: | |||
- name: Run tests | |||
run: npm test | |||
working-directory: server/ | |||
|
|||
- name: Linting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a separate job, possibly called Linting or something like that.
We don't want to attach jobs with different functionalities to the same job! This makes it easier to "fix" when it throws a failed test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes a lot of sense, there are a lot of steps in that job to begin with. I'll shift some of the other steps into seperate jobs as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will merge this CC
It looks good, just make sure to update your main branch to latest version and rebase/merge this branch on top of main to get rid of the merge conflicts! |
Not sure what is causing these errors... |
Well these errors are expected because our codebase does not follow the style guidelines that the linting job checks for. So it indicates that the linting works properly. |
Conflicts :(, once solved i will merge |
We will fix this later on our own :) |
Resolves #188