Skip to content
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

Introduce suspenders:lint generator #1148

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

stevepolitodesign
Copy link
Contributor

@stevepolitodesign stevepolitodesign commented Dec 1, 2023

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

  1. Install the gem from this branch.

    group :development, :test do
      gem "suspenders", github: "thoughtbot/suspenders", branch: "suspenders-3-0-0-stylelint"
    end
  2. Run the generator.

    bin/rails g suspenders:styles # <- This ensures we're linting the correct files 
    bin/rails g suspenders:lint
    
  3. Run the scripts and note the output.

    yarn run lint
    bundle exec rake standard
    

@stevepolitodesign stevepolitodesign changed the base branch from main to suspenders-3-0-0 December 1, 2023 20:22
Comment on lines +23 to +25
def configure_stylelint
copy_file "stylelintrc.json", ".stylelintrc.json"
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

json["scripts"] ||= {}

json["scripts"]["eslint"] = "npx eslint 'app/javascript/**/*.js'"
json["scripts"]["stylelint"] = "npx stylelint 'app/assets/stylesheets/**/*.css'"
Copy link
Contributor Author

@stevepolitodesign stevepolitodesign Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +27 to +29
def configure_eslint
copy_file "eslintrc.json", ".eslintrc.json"
end
Copy link
Contributor Author

@stevepolitodesign stevepolitodesign Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would an API only ever have JavaScript?

Copy link

@stevehanson stevehanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

json = JSON.parse content
json["scripts"] ||= {}

json["scripts"]["eslint"] = "npx eslint 'app/javascript/**/*.js'"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mike-burns mike-burns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool.

lib/generators/templates/lint/better_html_spec.rb Outdated Show resolved Hide resolved
test/test_helper.rb Outdated Show resolved Hide resolved
test/test_helper.rb Outdated Show resolved Hide resolved
lib/generators/suspenders/lint_generator.rb Outdated Show resolved Hide resolved
Comment on lines +57 to +58
Style/MultilineTernaryOperator:
Enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we allow this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Enabled: false
Style/MultilineTernaryOperator:
Enabled: false
Lint/UselessAssignment:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we allow this?

stevepolitodesign added a commit that referenced this pull request Dec 11, 2023
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`.
stevepolitodesign added a commit that referenced this pull request Dec 11, 2023
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`.
stevepolitodesign added a commit that referenced this pull request Dec 11, 2023
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
Copy link
Contributor Author

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

  1. 520834a introduces your recommendations.

  2. 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

  3. 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';
  4. f3b85a4 fixes a conflict between our stylelint config and prettier.

  5. 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.
@stevepolitodesign stevepolitodesign merged commit fe5fc93 into suspenders-3-0-0 Dec 11, 2023
2 checks passed
@stevepolitodesign stevepolitodesign deleted the suspenders-3-0-0-stylelint branch December 11, 2023 20:40
stevepolitodesign added a commit that referenced this pull request Dec 12, 2023
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
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
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 Mar 21, 2024
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.
stevepolitodesign added a commit that referenced this pull request Mar 21, 2024
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.
stevepolitodesign added a commit that referenced this pull request Mar 22, 2024
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.
stevepolitodesign added a commit that referenced this pull request Mar 22, 2024
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.
stevepolitodesign added a commit that referenced this pull request Mar 22, 2024
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.
stevepolitodesign added a commit that referenced this pull request Mar 22, 2024
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
stevepolitodesign added a commit that referenced this pull request Mar 22, 2024
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 added a commit that referenced this pull request Mar 22, 2024
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 added a commit that referenced this pull request Mar 22, 2024
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 added a commit that referenced this pull request May 10, 2024
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 added a commit that referenced this pull request May 10, 2024
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.
stevepolitodesign added a commit that referenced this pull request May 10, 2024
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 May 10, 2024
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.
stevepolitodesign added a commit that referenced this pull request May 10, 2024
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.
stevepolitodesign added a commit that referenced this pull request May 10, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants