-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
Setting up GitHub Action for CSS and future linting #1957
Setting up GitHub Action for CSS and future linting #1957
Conversation
I made this pull request a draft because I ran into some potential linting annoyances when working on issue #1958. I want to see if I can configure some of the style rules to stop the problem I ran into before merging. |
@averdin2 will make review steps |
@averdin2 Can we schedule a time to do a demo either on a Tuesday or Thursday meeting? |
@averdin2 Add on push back into linter config file. |
@averdin2 From testing, I had found that the linting config files needs to be in both the incoming branch and the merge into branch. Without the files existing in both, I get this error:
Can you confirm that this is the case? Also, the output from the linter is very useful, so I am wondering if the linter module provides a way of gathering or caching the data into a variable(preferred) or an artifact(less preferred)? That way, we can output it as a comment. If not, we might need a follow-up issue to add documentation on resolving linting issues. |
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.
Just one small architectural change. Everything else is awesome!
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.
Awesome work on this Alex! Just an additional thing after the backend meeting yesterday, we decided on a new structure for our Github actions. Click here for google sheet.
Basically linters will be on their own files (so SCSS linting will be on its own, JS linting on its own, etc.)
To reflect that change, could you edit the name of this action to something like "Lint SCSS" and rename the yml file to something like "lint-scss.yml"? Could you also update any other names in the workflow to reflect that?
Other than that, good job again on this.
@Aveline-art will contact @averdin2 regarding the changes for pull request. @Aveline-art will complete the changes if needed. |
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.
I tested this once more in my environment, and it looks good to me. I do agree with @Aveline-art about finding a way to logging the outputs into a comment, or writing a doc on how to resolve linting errors. However, I think that is something we can open as a new issue.
We discussed this part during the Tues meeting
Fixes #1441
What changes did you make and why did you make them?
Resources Used
Review Steps
Notes
VALIDATE_ALL_CODEBASE: false
means that the GitHub action only checks committed files to the repo. The reason why I am not checking the entire codebase is that I do not believe it is necessary to run a check on the entire codebase for every pull request. I made issue Make Style Linting Fixes #1958 to deal with all the errors that are in the current codebase as a result of runningVALIDATE_ALL_CODEBASE: true
. I believe after fixing these issues, only checking the files in new pull requests should be sufficient for linting the codebase.