Run every linter separately in Github Actions #5545

javierm · 2024-05-17T01:28:00Z

References

Closes Linters workflow for GitHub Actions fails for forks #5480 (although it doesn't properly fix it, this is a decent workaround)

Objectives

Run the linters on pull requests opened by external contributors
Avoid introducing syntax issues that are not detected by Pronto

app/components/proposals/form_component.html.erb

app/assets/javascripts/banners.js

app/models/setting.rb

app/assets/stylesheets/admin/menu.scss

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 added the Linters Rubocop, ERB Lint, ESLint, SCSS-Lint, ... label May 17, 2024

javierm self-assigned this May 17, 2024

javierm added this to Reviewing in Consul Democracy May 17, 2024

javierm moved this from Reviewing to Doing in Consul Democracy May 17, 2024

javierm force-pushed the linters_in_github_actions branch 3 times, most recently from 5766ba3 to 8bb589b Compare May 17, 2024 02:03