Introduce `suspenders:lint` generator by stevepolitodesign · Pull Request #1148 · thoughtbot/suspenders

stevepolitodesign · 2023-12-01T20:21:57Z

Closes #1107
Closes #1143

Creates a holistic linting solution that covers JavaScript, CSS, Ruby
and ERB.

Introduces eslint and stylelint NPM commands that leverage
@thoughtbot/eslint-config and @thoughtbot/stylelint-config.

yarn run eslint
yarn run stylelint

Also introduces .prettierrc based off of our Guides.

Introduces rake standard which also runs erblint to lint ERB files
via better_html, erb_lint and erblint-github.

A future commit will ensure these linting rules are run during CI.

It should be noted that we deliberately permit this generator to be
invoked on API only applications, because those applications can still
contain views, like ones used for mailers.

Also improves the developer experience by introducing with_test_suite
helper, allowing the caller to run the generator in an application using
minitest or RSpec.

To do

Run scripts that fix violations as part of the generator to ensure codebase is in a linted state.
Update styles generator by including a comment in the base styles to avoid Unexpected empty source linting error.

Testing manually

Install the gem from this branch.

group :development, :test do
  gem "suspenders", github: "thoughtbot/suspenders", branch: "suspenders-3-0-0-stylelint"
end

Run the generator.

bin/rails g suspenders:styles # <- This ensures we're linting the correct files 
bin/rails g suspenders:lint

Run the scripts and note the output.

yarn run lint
bundle exec rake standard

stevepolitodesign · 2023-12-01T20:27:21Z

lib/generators/suspenders/lint_generator.rb

+      def configure_stylelint
+        copy_file "stylelintrc.json", ".stylelintrc.json"
+      end


I wonder if we need this for API only applications? Mailers requires styles, but those are usually inline.

stevepolitodesign · 2023-12-01T20:28:22Z

lib/generators/templates/lint/erblint.rake

@seanpdoyle can we delete this? Looks like it was merged here?

lib/generators/templates/lint/eslint.json

stevepolitodesign · 2023-12-01T20:39:56Z

lib/generators/suspenders/lint_generator.rb

+        json["scripts"] ||= {}
+
+        json["scripts"]["eslint"] = "npx eslint 'app/javascript/**/*.js'"
+        json["scripts"]["stylelint"] = "npx stylelint 'app/assets/stylesheets/**/*.css'"


In #1145 we use postcss or tailwind, so there will be no scss files.

stevepolitodesign · 2023-12-01T20:49:53Z

lib/generators/suspenders/lint_generator.rb

+      def configure_eslint
+        copy_file "eslintrc.json", ".eslintrc.json"
+      end


Would an API only ever have JavaScript?

stevehanson

Amazing work! This will be such a valuable addition to Suspenders 🙌 I just did a quick review focused mostly on the JS linting setup. I left a few suggestions for how we name and execute the scripts. Let me know if you have any questions or wanted to pair on making those changes.

stevehanson · 2023-12-01T20:50:42Z

lib/generators/suspenders/lint_generator.rb

+        json = JSON.parse content
+        json["scripts"] ||= {}
+
+        json["scripts"]["eslint"] = "npx eslint 'app/javascript/**/*.js'"


This is awesome 🙌! I'm curious what your thoughts would be on aligning the script names and actions more closely with what is defined in the thoughtbot ESLint-config blog post? That post matches how we often define these on JavaScript projects, including in thoughtbelt (the React Native version of Suspenders). In particular:

defining linter scripts with names lint:{linter} (eg. lint:eslint, lint:stylelint, lint:prettier)

creating an additional lint script that runs all linters, since we will most commonly want to run all linters.

Adding a lint:prettier script, eg. prettier --check '**/*' --ignore-unknown

Do we want Prettier to run on JSON, CSS, and Markdown? It's my preference, but if not, we can restrict the glob a bit. The JSON formatting is definitely nice. I'm not sure offhand if we'll need CSS since we have stylelint. Markdown formatting is nice because it formats code in code blocks, but it can be a little annoying with tables, since it tries to line everything up.

Running eslint with the --max-warnings=0 flag. You can find more info about this in this section of the blog post, but basically, we want warnings to also fail the build

No need for npx in the eslint and stylelint commands. Yarn or whatever package manager is used will find these since they're dev dependencies.

I like to also have a fix:prettier command that runs Prettier and fixes any errors. CI would run lint:prettier. I rarely have to run fix:prettier in practice since my editor formats with Prettier on save, but it can be nice when we need to format files that were created by generators or for folks who don't have their editors configured with Prettier.

mike-burns

Cool.

lib/generators/templates/lint/better_html_spec.rb

test/test_helper.rb

lib/generators/suspenders/lint_generator.rb

mike-burns · 2023-12-01T21:08:34Z

lib/generators/templates/lint/erb-lint.yml

+      Style/MultilineTernaryOperator:
+        Enabled: false


Why do we allow this?

I might be misunderstanding, but we are not allowing this since Enabled is set to false.

mike-burns · 2023-12-01T21:10:03Z

lib/generators/templates/lint/erb-lint.yml

+        Enabled: false
+      Style/MultilineTernaryOperator:
+        Enabled: false
+      Lint/UselessAssignment:


Why do we allow this?

lib/generators/suspenders/lint_generator.rb

In #1148 we introduce `stylelint`. However, the empty styles introduced in #1145 are empty file violations. By adding comments, we avoid that error. Note that we need to add these comments because there's no way to automatically fix those violations with something like `prettier`.

In #1148 we introduce `stylelint`. However, the empty style sheets introduced in #1145 are empty file violations. By adding comments, we avoid that error. Note that we need to add these comments because there's no way to automatically fix those violations with something like `prettier`.

stevepolitodesign · 2023-12-11T18:11:59Z

@stevehanson after exhaustively testing this with a real application, I landed on this:

520834a introduces your recommendations.
c888832 adds --no-error-on-unmatched-pattern 'app/javascript/**/*.js' since new Rails applications don't necessarily ship with any .js files. Without this, we raise an error when running eslint

6390564 fixes violations where the linter could not determine where the imported file exists.

// app/javascript/application.js
// Configure your import map in config/importmap.rb. Read more: https://github.com/rails/importmap-rails
import '@hotwired/turbo-rails';
import 'controllers';

f3b85a4 fixes a conflict between our stylelint config and prettier.
5fe8cc4 accounts for this issue.

Closes #1107 Closes #1143 Creates a holistic linting solution that covers JavaScript, CSS, Ruby and ERB. Introduces [scripts][] that leverage [@thoughtbot/eslint-config][], [@thoughtbot/stylelint-config][] and [prettier][]. Also introduces `.prettierrc` based off of our [Guides][]. We need to pin `stylelint` and `@thoughtbot/stlyelint-config` to specific versions to account for this [open issue][]. Unfortunately, running `yarn run lint:stylelint` results in deprecation warnings, which will need to be addressed separately. [scripts]: https://docs.npmjs.com/cli/v6/using-npm/scripts [@thoughtbot/eslint-config]: https://github.com/thoughtbot/eslint-config [@thoughtbot/stylelint-config]: https://github.com/thoughtbot/stylelint-config [prettier]: https://prettier.io [Guides]: https://github.com/thoughtbot/guides/blob/main/javascript/README.md#formatting [open issue]: thoughtbot/stylelint-config#46 Introduces `rake standard` which also runs `erblint` to lint ERB files via [better_html][], [erb_lint][] and [erblint-github][]. [better_html]: https://github.com/Shopify/better-html [erb_lint]: https://github.com/Shopify/erb-lint [erblint-github]: https://github.com/github/erblint-github A future commit will ensure these linting rules are run during CI. In an effort to support that future commit, we ensure to run `yarn run fix:prettier` and `bundle exec standard:fix_unsafely` once the generator is complete. Otherwise, CI would fail because of linting violations. We call `standard:fix_unsafely` since `standard:fix` returns an error status code on new Rails applications. Running `standard:fix_unsafely` fixes this issue and returns a success status code. It should be noted that we deliberately permit this generator to be invoked on API only applications, because those applications can still contain views, like ones used for mailers. Also improves the developer experience by introducing `with_test_suite` helper, allowing the caller to run the generator in an application using minitest or RSpec.

When we introduced #1148 we did not test it against applications that invoked `suspenders:styles --css=tailwind`.

Follow-up to #1148 I noticed these tests did not fail even if `bin/rails standard` raised a linting violation. Because the upcoming `suspenders:ci` generator will run our linting tasks in CI, I felt having a test to do the same was no longer warranted.

Follow-up to #1145 and #1148 We decided it's best to limit the decisions a consumer needs to make. Even though we defaulted to PostCSS, providing an option to override this value felt like it went against the Suspenders ethos. Additionally, the current version of Suspenders uses PostCSS, so this change aligns with current behavior. Also removes linting rules around Tailwind.

Follow-up to #1148 and #1145 In #1148 and #1145, we introduce the need for yarn to manage dependencies. Those commits failed to establish a `.node-version` file, which [normally would be generated][1] by Rails if **not** using `import-maps`. This commit introduces that file, which compliments the existing `.ruby-version` file that is generated. I chose to use `.node-version` and not `.nvm` or `.tool-versions` to keep parity with Rails. The [current version][2] set by Rails is `18.15.0`, but a [future commit][3] aims to use the latest LTS value. This commit aims to use that version. This commit will also benefit a future `suspenders:ci` generator, since the `.node-version` file will be used in CI. [1]: rails/rails#51393 [2]: https://github.com/rails/rails/blob/e8638c9a942e94f097dc8f37a3b58ac067a5ca16/railties/lib/rails/generators/app_base.rb#L18 [3]: rails/rails#51393

Follow-up to #1148 and #1145 In #1148 and #1145, we introduce the need for yarn to manage dependencies. Those commits failed to establish a `.node-version` file, which [normally would be generated][1] by Rails if **not** using `import-maps`. This commit introduces that file, which compliments the existing `.ruby-version` file that is generated. I chose to use `.node-version` and not `.nvm` or `.tool-versions` to keep parity with Rails. The [current version][2] set by Rails is `18.15.0`, but a [future commit][3] aims to use the latest LTS value. This commit aims to use that version. This commit will also benefit a future `suspenders:ci` generator, since the `.node-version` file will be used in CI. [1]: https://github.com/rails/rails/blob/68b20b6513fe56ca80e4966628c231b4d6113bea/railties/lib/rails/generators/rails/app/app_generator.rb#L57-L59 [2]: https://github.com/rails/rails/blob/e8638c9a942e94f097dc8f37a3b58ac067a5ca16/railties/lib/rails/generators/app_base.rb#L18 [3]: rails/rails#51393

In #1148 we introduce `stylelint`. However, the empty style sheets introduced in #1145 are empty file violations. By adding comments, we avoid that error. Note that we need to add these comments because there's no way to automatically fix those violations with something like `prettier`.

Closes #1107 Closes #1143 Creates a holistic linting solution that covers JavaScript, CSS, Ruby and ERB. Introduces [scripts][] that leverage [@thoughtbot/eslint-config][], [@thoughtbot/stylelint-config][] and [prettier][]. Also introduces `.prettierrc` based off of our [Guides][]. We need to pin `stylelint` and `@thoughtbot/stlyelint-config` to specific versions to account for this [open issue][]. Unfortunately, running `yarn run lint:stylelint` results in deprecation warnings, which will need to be addressed separately. [scripts]: https://docs.npmjs.com/cli/v6/using-npm/scripts [@thoughtbot/eslint-config]: https://github.com/thoughtbot/eslint-config [@thoughtbot/stylelint-config]: https://github.com/thoughtbot/stylelint-config [prettier]: https://prettier.io [Guides]: https://github.com/thoughtbot/guides/blob/main/javascript/README.md#formatting [open issue]: thoughtbot/stylelint-config#46 Introduces `rake standard` which also runs `erblint` to lint ERB files via [better_html][], [erb_lint][] and [erblint-github][]. [better_html]: https://github.com/Shopify/better-html [erb_lint]: https://github.com/Shopify/erb-lint [erblint-github]: https://github.com/github/erblint-github A future commit will ensure these linting rules are run during CI. In an effort to support that future commit, we ensure to run `yarn run fix:prettier` and `bundle exec standard:fix_unsafely` once the generator is complete. Otherwise, CI would fail because of linting violations. We call `standard:fix_unsafely` since `standard:fix` returns an error status code on new Rails applications. Running `standard:fix_unsafely` fixes this issue and returns a success status code. It should be noted that we deliberately permit this generator to be invoked on API only applications, because those applications can still contain views, like ones used for mailers. However, a future commit could explore removing the JavaScript linters. Also improves the developer experience by introducing `with_test_suite` helper, allowing the caller to run the generator in an application using minitest or RSpec.

When we introduced #1148 we did not test it against applications that invoked `suspenders:styles --css=tailwind`.

Follow-up to #1148 I noticed these tests did not fail even if `bin/rails standard` raised a linting violation. Because the upcoming `suspenders:ci` generator will run our linting tasks in CI, I felt having a test to do the same was no longer warranted.

Follow-up to #1145 and #1148 We decided it's best to limit the decisions a consumer needs to make. Even though we defaulted to PostCSS, providing an option to override this value felt like it went against the Suspenders ethos. Additionally, the current version of Suspenders uses PostCSS, so this change aligns with current behavior. Also removes linting rules around Tailwind.

Follow-up to #1148 and #1145 In #1148 and #1145, we introduce the need for yarn to manage dependencies. Those commits failed to establish a `.node-version` file, which [normally would be generated][1] by Rails if **not** using `import-maps`. This commit introduces that file, which compliments the existing `.ruby-version` file that is generated. I chose to use `.node-version` and not `.nvm` or `.tool-versions` to keep parity with Rails. The [current version][2] set by Rails is `18.15.0`, but a [future commit][3] aims to use the latest LTS value. This commit aims to use that version. This commit will also benefit a future `suspenders:ci` generator, since the `.node-version` file will be used in CI. [1]: https://github.com/rails/rails/blob/68b20b6513fe56ca80e4966628c231b4d6113bea/railties/lib/rails/generators/rails/app/app_generator.rb#L57-L59 [2]: https://github.com/rails/rails/blob/e8638c9a942e94f097dc8f37a3b58ac067a5ca16/railties/lib/rails/generators/app_base.rb#L18 [3]: rails/rails#51393

stevepolitodesign changed the base branch from main to suspenders-3-0-0 December 1, 2023 20:22

stevepolitodesign commented Dec 1, 2023

View reviewed changes

lib/generators/templates/lint/eslint.json Outdated Show resolved Hide resolved

stevepolitodesign force-pushed the suspenders-3-0-0-stylelint branch from 8c944e5 to 955cf84 Compare December 1, 2023 20:30

stevepolitodesign requested review from seanpdoyle and stevehanson December 1, 2023 20:31

stevepolitodesign marked this pull request as ready for review December 1, 2023 20:31

stevepolitodesign requested a review from thiagoa as a code owner December 1, 2023 20:31

stevepolitodesign mentioned this pull request Dec 1, 2023

Suspenders 3.0.0 #1135

Merged

17 tasks

stevepolitodesign commented Dec 1, 2023

View reviewed changes

stevehanson reviewed Dec 1, 2023

View reviewed changes

mike-burns reviewed Dec 1, 2023

View reviewed changes

stevepolitodesign commented Dec 4, 2023

View reviewed changes

lib/generators/suspenders/lint_generator.rb Outdated Show resolved Hide resolved

stevepolitodesign force-pushed the suspenders-3-0-0-stylelint branch from 3d1e441 to 626ddbb Compare December 5, 2023 21:33

stevepolitodesign mentioned this pull request Dec 11, 2023

Fix stylelint violations #1150

Merged

stevepolitodesign force-pushed the suspenders-3-0-0-stylelint branch from 626ddbb to 5fe8cc4 Compare December 11, 2023 18:02

stevepolitodesign requested a review from stevehanson December 11, 2023 18:12

stevepolitodesign force-pushed the suspenders-3-0-0-stylelint branch from 2a6ef3e to 9743a2d Compare December 11, 2023 20:18

stevepolitodesign force-pushed the suspenders-3-0-0-stylelint branch from 9743a2d to 439b3ee Compare December 11, 2023 20:20

stevepolitodesign merged commit fe5fc93 into suspenders-3-0-0 Dec 11, 2023

stevepolitodesign deleted the suspenders-3-0-0-stylelint branch December 11, 2023 20:40

This was referenced Dec 11, 2023

WIP Introduce suspenders:lint generator #1143

Closed

Lint HTML Templates with ERBLint and BetterHtml #1107

Closed

stevepolitodesign added a commit that referenced this pull request Dec 12, 2023

Fix stylelint violations when using Tailwind

8830c94

When we introduced #1148 we did not test it against applications that invoked `suspenders:styles --css=tailwind`.

stevepolitodesign added a commit that referenced this pull request Dec 12, 2023

Fix stylelint violations when using Tailwind

23505db

When we introduced #1148 we did not test it against applications that invoked `suspenders:styles --css=tailwind`.

stevepolitodesign mentioned this pull request Dec 12, 2023

Fix stylelint violations when using Tailwind #1153

Merged

stevepolitodesign added a commit that referenced this pull request Dec 12, 2023

Fix stylelint violations when using Tailwind (#1153)

8628dcb

When we introduced #1148 we did not test it against applications that invoked `suspenders:styles --css=tailwind`.

stevepolitodesign mentioned this pull request Mar 21, 2024

Lint Generator: Remove generated test/spec files #1167

Merged

stevepolitodesign mentioned this pull request Mar 22, 2024

Styles Generator: Drop support for Tailwind #1169

Merged

stevepolitodesign mentioned this pull request Mar 22, 2024

Introduce suspenders:prerequisites generator #1171

Merged

stevepolitodesign added a commit that referenced this pull request May 10, 2024

Fix stylelint violations when using Tailwind (#1153)

8fb94d2

When we introduced #1148 we did not test it against applications that invoked `suspenders:styles --css=tailwind`.

stevepolitodesign mentioned this pull request Jun 18, 2024

How do we want to handle Ruby linting now that Rails ships with a solution? #1214

Closed

Uh oh!

Conversation

stevepolitodesign commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To do

Testing manually

Uh oh!

stevepolitodesign Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

stevepolitodesign Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stevepolitodesign Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevepolitodesign Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevehanson left a comment

Choose a reason for hiding this comment

Uh oh!

stevehanson Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

mike-burns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mike-burns Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

stevepolitodesign Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

mike-burns Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stevepolitodesign commented Dec 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevepolitodesign commented Dec 1, 2023 •

edited

Loading

stevepolitodesign Dec 1, 2023 •

edited

Loading

stevepolitodesign Dec 1, 2023 •

edited

Loading