rubocop / rubocop-sequel Goto Github PK

There are a couple of failing rubocops on the latest version of this codebase. Apparently these aren't being run in CI so they went unnoticed.

Ignores options passed in an options hash along with gin_trgm_ops relevant with pg_trgm extension.
This way concurrently: true, though provided, is not detected by linter.

Sequel.migration do
  no_transaction

  up do
    DB.execute('CREATE EXTENSION IF NOT EXISTS pg_trgm;')
    alter_table(:foos) do
      opts = {
        name: :foos_bar_search_index,
        type: :gin,
        concurrently: true
      }
      add_index(Sequel.lit(" gin_trgm_ops"), opts)
    end
  end
end

Issue produced with the following version

GIT
  remote: git://github.com/rubocop-hq/rubocop-sequel.git
  revision: 99fc07d33c6dd4a2b38627316764ab2cc7ce750c
  ref: 99fc07d33c6dd4a2b38627316764ab2cc7ce750c
  specs:
    rubocop-sequel (0.1.0)
      rubocop (~> 1.0)

A migration file that uses migration methods like set_column_type, or other non-reversible ones mentioned here https://sequel.jeremyevans.net/rdoc/files/doc/migration_rdoc.html#label-A+Basic+Migration is not reversible automatically, and should require specific up/down methods in migrations in projects where reversibility is important. I imagine a rubocop rule could check for this.

Curious if maintainers would merge a PR that adds a new cop for this?

Hi rubocop & sequel users and rubocop-sequel maintainers!

We've recently faced an issue with partial indexes using boolean field.

Basically, when index has condition WHERE field and sequel generates (where(field: true) as WHERE field IS TRUE, index isn't used, by postgres regardless of the nullability of the field. The same is true vice-versa. This is the expected behavior, but we only faced it recently when we discovered that partial index can help us. Before, we didn't pay attention to which conditions we used and used them interchangeably (and it didn't create a problem because of not-nullable columns).

What I think would be great in rubocop-sequel is to enforce boolean conditions to one style:
either you use where(field: true), where { field =~ true } and where: Sequel[:field] =~ true,
or you use where(:field), where { field } and where: Sequel[:field].

The former is preferred and should be default because of null handling, but one may have a different setup, so I guess having it configurable might make sense.

I think such cop may help with different kinds of related problems.

Do you think it make sense to have such cop?

I do not believe this cop should be auto-correctable.

From the docs on Model#save_changes, "If the object has not been modified, returns nil".

My code relied on the fact that Model#save returns self if successful, even if no changes were made. When auto-corrected my tests broke.

Bundler could not find compatible versions for gem "rubocop":
  In Gemfile:
    rubocop (~> 1.2.0)

    rubocop-sequel was resolved to 0.0.6, which depends on
      rubocop (>= 0.55, ~> 0.55)

Is there any work that needs to be done to support the >= 1.0.0 releases?

There are some breaking changes listed in rubocop changelog. Does this gem need any code changes or just update to the gemspec?

https://github.com/rubocop-hq/rubocop-sequel/blob/7099c0bd3ea5c04a2e3dadb7c0f63fa18f2c3436/rubocop-sequel.gemspec#L18

Thanks!

Hi! wonder if you're gonna cut or release a new version for Ruby 3 support. Thanks!

I got the cop RuboCop(Sequel/ConcurrentIndex) complaining when I'm trying to run a migration to create/drop an index on a partitioned table in PostgreSQL when concurrently: true is not set, while it is currently not possible in PostgreSQL.

Steps to reproduce

1. Create a migration that adds an index to a partitioned table
2. Don't add concurrently: true
3. Check that the cop RuboCop(Sequel/ConcurrentIndex) is complaining
4. Add concurrently: true
5. Run the migration
6. Get an exception

Sequel::DatabaseError: PG::FeatureNotSupported: ERROR:  cannot create index on partitioned table "my_table" concurrently

ROM::SQL.migration do
  no_transaction

  change do
    alter_table :my_table do
      add_index :my_index, concurrently: true
    end
  end
end

Expected behavior

The cop should not complain, as the proposed change is not working for partitioned table in PostgreSQL

Actual behavior

The cop is complaining, forcing me to change to a non-working solution where a Sequel::DatabaseError: PG::FeatureNotSupported is raised.

Proposed change

If this situation is still happening with up-to-date gems and ruby versions, I believe we could go in 2 directions:

• Remove the cop: fast and simple, but might decrease experience for people that don't fact this issue.
• Make the cop less restrictive or more clear: we could enforce to specify concurrently, even if it is set to false, and change the message from 'Prefer creating or dropping new index concurrently' to 'Specify concurrently option when creating or dropping an index.
• I'm of course open for other solutions!

I'm more in favor of the second option of keeping the cop but forcing it to say we should specify the concurrently option, instead of setting it to true, but I'm open for suggestions!

I've opened a PR for this if it makes sense.

System configuration

rubocop-sequel version:
0.3.4

sequel version:
5.76.0

pg version:
1.5.4

Ruby version:
ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]

I'm a bit puzzled why this is a cop. I can imagine theoretical reasons, but not particularly practical ones, where the gains outweigh the costs (more nullable columns, being unable to use change blocks)

Hey there!

I've recently created https://github.com/rubocop-hq to serve as the new home of RuboCop and related projects (see https://speakerdeck.com/bbatsov/all-about-rubocop-rubykaigi-2018?slide=157 for some background on this). I'd love to see rubocop-sequel live under the same organization as RuboCop itself.

Let me know if you're interested in doing this and we'll hash out the details later.