-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Run every linter separately in Github Actions #5545
Open
javierm
wants to merge
10
commits into
master
Choose a base branch
from
linters_in_github_actions
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javierm
force-pushed
the
linters_in_github_actions
branch
3 times, most recently
from
May 17, 2024 02:03
5766ba3
to
8bb589b
Compare
javierm
force-pushed
the
linters_in_github_actions
branch
8 times, most recently
from
May 17, 2024 03:00
702a620
to
dbfa9f9
Compare
javierm
force-pushed
the
linters_in_github_actions
branch
from
May 17, 2024 03:09
dbfa9f9
to
33d7407
Compare
In commit 9329e4b, `try` was added becaus there was a case where this partial was rendered and the `current_booth` method didn't exist. However, that's no longer the case since commit d5c7858. Since then, this partial is only rendered in the officing section, where the `current_booth` method is defined. So we can use the safe navigation operator instead of `try`.
We're excluding these files because they use `raw` to render content than only administrators can edit, and we trust administrators not to provide unsafe HTML. We should definitely sanitize them at some point but, at the same time, we should also try to keep compatibility in installations taking advantage of `raw`. Also note that ERB Lint does not allow customizing the severity of a linter; if it ever does, we'll use the severity rule instead of excluding files.
Since now we aren't just using ESLint in the context of a pull request (with pronto-eslint), we're also adding the `ignorePatterns` option so it ignores third-party files. Note we're using ESLint 8 because ESLint 9 doesn't support our `.eslintrc` configuration file, which we need because it's required by pronto-eslint. However, when running pronto-eslint, we're using ESLint 6 (I think); this means that now the eslint command we run in development and the one run by pronto might behave differently, particularly if we add rules that have been introduced in ESLint 7 or 8. For now, since we generated the `.eslintrc` file using ESLint 6.0.1, everything works as expected in both situations.
This rule is part of the `eslint:recommended` package. It was the only rule we weren't following 100% of the time (other than the line length).
This file was generated by a different developer who ignored the nesting rules. Refactoring it has been in our TODO list for years, but we haven't done it yet. Since we want stylelint to check that there are no errors after the changes in a pull request, but we've still got errors in this file, we're ignoring them for now.
This way we can ignore the selector-class-pattern stylelint rule error due to the `.ssb-whatsapp_app` selector.
We can't simply remove the nesting selector in this case since it's inside a `:not` pseudo-class. Removing the nesting selector would cause a syntaxy error.
We're going to add a linters workflow, so the name would be confusing.
The main reason for this change is that Pronto doesn't run in pull requests opened by external contributors, and we haven't found how to fix this issue. Another reason is that Pronto only detects issues in the lines where the changes are done, but sometimes we introduce issues related to other lines and those aren't detected by Pronto. Also, when adding or changing Rubocop rules, or when we update Rubocop, Pronto doesn't check whether those rules are applied in every Ruby and ERB file, and we sometimes forget to do so (particularly in ERB files). So, from now, the linters will check all the code in every pull request.
javierm
force-pushed
the
linters_in_github_actions
branch
from
May 17, 2024 20:28
33d7407
to
5724d86
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives