Add standardrb and generate a todo file by bagedevimo · Pull Request #215 · NZOI/nztrain

bagedevimo · 2023-12-23T08:59:03Z

Brief discussion on Discord garnered no hatred for the idea of using a
linter, and it can be aspirational - we move towards standard slowly :)

bagedevimo · 2023-12-23T08:59:14Z

Current dependencies on/for this PR:

master
- PR Add standardrb and generate a todo file #215 👈

This stack of pull requests is managed by Graphite.

tom93

FYI, the deps are broken (ruby 2.3.8):

$ bundle install
Resolving dependencies...
Bundler could not find compatible versions for gem "rack":
  In snapshot (Gemfile.lock):
    rack (= 1.6.13)

  In Gemfile:
    rails (= 4.1.16) was resolved to 4.1.16, which depends on
      actionpack (= 4.1.16) was resolved to 4.1.16, which depends on
        rack (~> 1.5.2)

    better_errors was resolved to 2.9.1, which depends on
      rack (>= 0.9.0)

    capybara was resolved to 2.17.0, which depends on
      rack (>= 1.0.0)

    sinatra was resolved to 1.4.8, which depends on
      rack (~> 1.5)

Running `bundle update` will rebuild your snapshot from scratch, using only
the gems in your Gemfile, which may resolve the conflict.

bagedevimo · 2023-12-23T10:10:54Z

yep, my environment is getting too confusing, so i'll wait for some more stuff to merge. it also needs rails 4.2 and ruby 2.4 so may as well just wait for those.

tom93 · 2023-12-23T10:24:27Z

I wanted to try it out, it worked for me when I installed it using bundle add standard -g development,test (it picked standard 0.2.5).

bagedevimo · 2023-12-23T10:26:02Z

oh well fuck, force push over this branch?

tom93 · 2023-12-23T10:26:42Z

Wait what?

bagedevimo · 2023-12-23T10:27:28Z

haha sorry, i just mean i couldn't get it working on ruby 2.3 properly. so feel free to commit your changes to this branch and force-push over my commit

tom93 · 2023-12-23T10:36:16Z

I just posted that in case other people wanted to try standard, but I guess we might as well switch to the old version instead of waiting.
(I'll also switch from "standardrb" to "standard" if that's okay -- the first is an alias for the second, their docs use the second and say the "rb" suffix is just because there was an existing CLI tool with the name "standard".)

bagedevimo · 2023-12-23T10:37:39Z

please do - i just have it called "standardrb" in my head because we also use "standardjs" at work :)

tom93 · 2023-12-23T10:51:22Z

On my trivial unrelated PR (#216), standard 0.2.5 suggested if !... => unless ... while standard 1.32.1 didn't complain.
Do you have a preference in this particular case?
And more generally, should we skip the older version of standard to reduce formatting changes?
(We could manually install a new version of standard using a new version of Ruby, without going through the Gemfile and bundler.)

Brief discussion on Discord garnered no hatred for the idea of using a linter, and it can be aspirational - we move towards standard slowly :) (The todo file ignores violations in existing files, to allow gradual migration. It was generated using `standardrb --generate-todo` and is normally called .standard_todo.yml, but we have to rename it to .standard.yml for now for compatibility with older versions of standrad that don't support the _todo.yml file.) Co-authored-by: Tom Levy <tomlevy93@gmail.com>

bagedevimo · 2023-12-23T12:09:35Z

i'm pretty ambivalent, i doubt we're going to fix everything in _todo.yml before we can update standardrb

tom93

I force-pushed some changes:

1.0.5 => 0.2.5 (for Ruby 2.3 compat)
.standard_todo.yml => .standard.yml (since _todo.yml requires 0.4.0; added a TODO comment to rename)
standardrb => standard (as discussed)

tom93 · 2024-01-01T09:10:00Z

A little late, but should we also add the standard-rails gem?

bagedevimo · 2024-01-04T10:58:28Z

Unfortunately not - I would have if I could - it has a dependency on ruby 2.6 https://rubygems.org/gems/standard-rails/versions/0.0.1

This workflow runs the tests and is a replacement for our previous Travis CI setup (.travis.yml), which we no longer use due to #147. The commands to run the install scripts and the tests are loosely based on .travis.yml, with several updates and fixes. The workflow's structure as a whole is based on GitHub Action's Ruby on Rails CI template. The most notable change compared to .travis.yml is that we are dropping the `--skip-update` option when running install.bash. That option skips a couple of steps: `bundle install`, migrate.bash, whenever.bash. The new workflow can work without those steps, but it's fragile: the `bundle install` step is only safe to skip because we pass `bundler-cache: true` to ruby/setup-ruby; if we disable the cache then ruby/setup-ruby won't run `bundle install` and the build will fail. So it's better to always run `bundle install` explicitly (running it twice doesn't do any harm, and it's very fast the second time). Dropping `--skip-updates` also causes migrate.bash to be executed, which is a good thing because it exercises the migration code. It also allows us to drop the `rake db:test:load` step. (Creating the database by loading the schema is fast, but db/schema.rb is slightly incomplete: it doesn't include the index "index_users_on_username" because of a Rails limitation. Since we are running the migrations anyway, we might as well use the database created by them.) Later on we will also add a check that reports an error if the schema dumped after running the migrations has any differences compared to the schema in the repository (#245). Running whenever.bash is quick and doesn't do any harm. Another notable change is that we are adding a "lint" job that runs "standardrb" (recently added in #215). Currently this doesn't do much because .standard.yml ignores all the files. There are a couple of differences compared to the GitHub Actions Ruby on Rails CI template[1]: - The template only runs the job on push/pull-request for branch "master", but we run the job for all branches because we want to be able to push commits and have them tested without having to open a pull request. - The template runs on ubuntu-latest, but we use ubuntu-20.04 because the build currently fails on ubuntu-latest. - The template doesn't have a Docker health check for the "postgres" service (to wait until it starts). We use a health check based on [2], but with a slightly different command[3]. - We have a "redis" service (based on [4]). - We have a custom install script. - The template uses bin/rails etc., which we don't have yet. - The template uses db:schema:load, which we don't need (see discussion of db:test:load above). - The template has pretty step names, which we don't bother with. - The template's "lint" job uses bundler-audit, brakeman, and rubocop. We use standardrb (which wraps rubocop), and will add bundler-audit later (#244). [1] https://github.com/actions/starter-workflows/blob/c31fe3d5d44d7cb4c912f4c3213f7b4610f13ea2/ci/rubyonrails.yml [2] https://docs.github.com/en/actions/using-containerized-services/creating-postgresql-service-containers#running-jobs-directly-on-the-runner-machine [3] We pass `-U postgres` to pg_isready to suppress error messages: pg_isready works even if the username is invalid, however it causes the following error message to appear repeatedly in the postgres container log (displayed in the "Stop containers" step): `FATAL: role "root" does not exist` [4] https://docs.github.com/en/actions/using-containerized-services/creating-redis-service-containers#running-jobs-directly-on-the-runner-machine Co-authored-by: Tom Levy <tomlevy93@gmail.com>

This workflow runs the tests and is a replacement for our previous Travis CI setup (.travis.yml), which we no longer use due to #147. The commands to run the install scripts and the tests are loosely based on .travis.yml, with several updates and fixes. The overall structure of the workflow is based on the GitHub Actions Ruby on Rails CI template[1]. The most notable change compared to .travis.yml is that we are dropping the `--skip-update` option when running install.bash. That option skips a couple of steps: `bundle install`, migrate.bash, whenever.bash. The new workflow can run without those steps, but it's fragile: the `bundle install` step is only safe to skip because we pass `bundler-cache: true` to ruby/setup-ruby; if we disable the cache then ruby/setup-ruby won't run `bundle install` and the build will fail. So it's better to always run `bundle install` explicitly (running it twice doesn't do any harm, and it's very fast the second time). Dropping `--skip-updates` also causes migrate.bash to be executed, which is a good thing because it exercises the migration code. It also allows us to drop the `rake db:test:load` step. (Creating the database by loading the schema is fast, but db/schema.rb is slightly incomplete: it doesn't include the index "index_users_on_username" because of a Rails limitation. Since we are running the migrations anyway, we might as well use the database created by them.) Later on we will also add a check that reports an error if the schema dumped after running the migrations has any differences compared to the schema in the repository (#245). Running whenever.bash is quick and doesn't do any harm. Another notable change is that we are adding a "lint" job that runs "standardrb" (recently added in #215). Currently this doesn't do much because .standard.yml ignores all the files. Compared to the GitHub Actions Ruby on Rails CI template[1], there are a couple of differences: - The template only runs the job on push/pull-request for branch "master", but we run the job for all branches because we want to be able to push commits and have them tested without having to open a pull request. - The template runs on ubuntu-latest, but we use ubuntu-20.04 because the build currently fails on ubuntu-latest. - The template doesn't have a Docker health check for the "postgres" service (to wait until it starts). We use a health check based on [2], but with a slightly different command[3]. - We have a "redis" service (based on [4]). - We have a custom install script. - The template uses bin/rails etc., which we don't have yet. - The template uses db:schema:load, which we don't need (see discussion of db:test:load above). - The template has pretty step names, which we don't bother with. - The template's "lint" job uses bundler-audit, brakeman, and rubocop. We use standardrb (which wraps rubocop), and will add bundler-audit later (#244). [1] https://github.com/actions/starter-workflows/blob/c31fe3d5d44d7cb4c912f4c3213f7b4610f13ea2/ci/rubyonrails.yml [2] https://docs.github.com/en/actions/using-containerized-services/creating-postgresql-service-containers#running-jobs-directly-on-the-runner-machine [3] We pass `-U postgres` to pg_isready to suppress error messages: pg_isready works even if the username is invalid, however it causes the following error message to appear repeatedly in the postgres container log (displayed in the "Stop containers" step): `FATAL: role "root" does not exist` [4] https://docs.github.com/en/actions/using-containerized-services/creating-redis-service-containers#running-jobs-directly-on-the-runner-machine Co-authored-by: Tom Levy <tomlevy93@gmail.com>

tom93 reviewed Dec 23, 2023

View reviewed changes

tom93 force-pushed the 12-23-Add_standardrb_and_generate_a_todo_file branch from 4e5fa76 to b078f21 Compare December 23, 2023 11:09

tom93 mentioned this pull request Dec 23, 2023

Remove unused ruby_parser gem #217

Merged

tom93 approved these changes Dec 23, 2023

View reviewed changes

bagedevimo marked this pull request as ready for review December 23, 2023 21:05

bagedevimo merged commit df085d0 into master Dec 23, 2023

bagedevimo deleted the 12-23-Add_standardrb_and_generate_a_todo_file branch December 23, 2023 21:06

tom93 mentioned this pull request Jan 4, 2024

Add standard-rails gem once we update to Ruby 2.6 #239

Open

Conversation

bagedevimo commented Dec 23, 2023

Uh oh!

bagedevimo commented Dec 23, 2023

Uh oh!

tom93 left a comment

Choose a reason for hiding this comment

Uh oh!

bagedevimo commented Dec 23, 2023

Uh oh!

tom93 commented Dec 23, 2023

Uh oh!

bagedevimo commented Dec 23, 2023

Uh oh!

tom93 commented Dec 23, 2023

Uh oh!

bagedevimo commented Dec 23, 2023

Uh oh!

tom93 commented Dec 23, 2023

Uh oh!

bagedevimo commented Dec 23, 2023

Uh oh!

tom93 commented Dec 23, 2023

Uh oh!

bagedevimo commented Dec 23, 2023

Uh oh!

tom93 left a comment

Choose a reason for hiding this comment

Uh oh!

tom93 commented Jan 1, 2024

Uh oh!

bagedevimo commented Jan 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants