Added Rubocop config and GitHub Actions configuration#73
Added Rubocop config and GitHub Actions configuration#73igor-alexandrov wants to merge 3 commits intorkrage:masterfrom
Conversation
|
@rkrage wow, it seems I already started to work on GitHub Actions. I will update this PR. |
igor-alexandrov
force-pushed
the
chore/rubocop
branch
from
e4059ff to
1a72555
Compare
|
@rkrage this is ready to be merged. I will add more Actions in a separate commit to transition from CircleCI. |
|
@igor-alexandrov can you rebase this and resolve the merge conflict? |
Rakefile
Outdated
| "default" | ||
| else | ||
| gemfile | ||
| end |
There was a problem hiding this comment.
I would prefer if this looked like:
output_prefix = if gemfile.empty? || gemfile == "Gemfile"
"default"
else
gemfile
endThere was a problem hiding this comment.
I prefer this style too, updated to pass Rubocop rules to:
output_prefix =
if gemfile.empty? || gemfile == "Gemfile"
"default"
else
gemfile
end
| attr_reader :config, :cache | ||
|
|
||
| def configure(&blk) | ||
| blk.call(config) |
There was a problem hiding this comment.
I prefer the explicitness of blk.call over yield whenever possible
There was a problem hiding this comment.
Reverted to blk.call, but seems that yield is faster: https://github.com/fastruby/fast-ruby#proccall-and-block-arguments-vs-yieldcode
lib/pg_party/adapter_decorator.rb
Outdated
| "INCLUDING ALL" | ||
| else | ||
| 'INCLUDING ALL EXCLUDING INDEXES' | ||
| "INCLUDING ALL EXCLUDING INDEXES" |
There was a problem hiding this comment.
I guess I missed this one previously, but I would prefer if everything was moved over to the left like I mentioned in another comment above
There was a problem hiding this comment.
Updated to have the similar style as in the comment above.
like_option =
if !partition_type || create_with_pks
"INCLUDING ALL"
else
"INCLUDING ALL EXCLUDING INDEXES"
end
lib/pg_party/adapter_decorator.rb
Outdated
| if table_partitioned?(table_name) | ||
| add_index_only(table_name, type: index_type, name: updated_name, using: using, columns: index_columns, | ||
| options: index_options) | ||
| options: index_options) |
There was a problem hiding this comment.
I think each keyword arg should be on its own line to look like the create_table_like calls above
There was a problem hiding this comment.
Completely agree, updated.
lib/pg_party/adapter_decorator.rb
Outdated
| _created_index_names << updated_name # Track as created before execution of concurrent index command | ||
| add_index_from_options(table_name, name: updated_name, type: index_type, algorithm: algorithm, using: using, | ||
| columns: index_columns, options: index_options) | ||
| columns: index_columns, options: index_options) |
lib/pg_party/adapter_decorator.rb
Outdated
| "SELECT relname FROM pg_class, pg_index WHERE pg_index.indisvalid = false AND "\ | ||
| "pg_index.indexrelid = pg_class.oid AND relname = #{quote(index_name)}" | ||
| "SELECT relname FROM pg_class, pg_index WHERE pg_index.indisvalid = false AND " \ | ||
| "pg_index.indexrelid = pg_class.oid AND relname = #{quote(index_name)}" |
There was a problem hiding this comment.
This one should probably just use heredoc
lib/pg_party/model_decorator.rb
Outdated
| end | ||
|
|
||
| alias_method :hash_partition_key_in, :list_partition_key_in | ||
| alias hash_partition_key_in list_partition_key_in |
There was a problem hiding this comment.
Not sure how I feel about this one...
There was a problem hiding this comment.
Reverted back alias_method and updated Rubocop configuration to make alias_method preferred over alias.
| algorithm: :concurrently, | ||
| where: "created_at > '#{current_date.to_time.iso8601}'" | ||
| algorithm: :concurrently, | ||
| where: "created_at > '#{current_date.to_time.iso8601}'" |
There was a problem hiding this comment.
Updated to match style, described above.
spec/integration/migration_spec.rb
Outdated
| in_threads: index_threads, algorithm: :concurrently, | ||
| where: "created_at > '#{current_date.to_time.iso8601}'" | ||
| in_threads: index_threads, algorithm: :concurrently, | ||
| where: "created_at > '#{current_date.to_time.iso8601}'" |
spec/integration/migration_spec.rb
Outdated
|
|
||
| adapter.add_index_on_all_partitions table_name, "#{table_name}_id", name: index_prefix, | ||
| in_threads: index_threads, algorithm: :concurrently, unique: true | ||
| in_threads: index_threads, algorithm: :concurrently, unique: true |
There was a problem hiding this comment.
Don't love this too, fixed.
spec/integration/migration_spec.rb
Outdated
| before do | ||
| allow(Parallel).to receive(:map).with([child_table_name, sibling_table_name], in_threads: index_threads) | ||
| .and_yield(child_table_name).and_yield(sibling_table_name) | ||
| .and_yield(child_table_name).and_yield(sibling_table_name) |
There was a problem hiding this comment.
Seems like all of these would look better if we just move the .with call to its own line
spec/integration/migration_spec.rb
Outdated
| ) | ||
| "`in_threads:` cannot be used within a transaction. If running in a migration, use " \ | ||
| "`disable_ddl_transaction!` and break out this operation into its own migration." | ||
| ) |
spec/integration/migration_spec.rb
Outdated
| 'in_threads: must be lower than your database connection pool size' | ||
| ) | ||
| "in_threads: must be lower than your database connection pool size" | ||
| ) |
spec/integration/threading_spec.rb
Outdated
| "success" | ||
| else | ||
| "failed" | ||
| end |
There was a problem hiding this comment.
Same comment about if statements above
There was a problem hiding this comment.
Updated to match overall style
spec/integration/threading_spec.rb
Outdated
| "success" | ||
| else | ||
| "failed" | ||
| end |
There was a problem hiding this comment.
Same comment about if statements above
.github/workflows/lint_code.yml
Outdated
| run: | | ||
| gem install bundler | ||
| bundle install --jobs 4 --retry 3 | ||
| bundle exec rubocop No newline at end of file |
There was a problem hiding this comment.
Looks like we need a newline here
There was a problem hiding this comment.
Added newline
igor-alexandrov
force-pushed
the
chore/rubocop
branch
2 times, most recently
from
f0a8e64 to
bbb91e9
Compare
Rakefile
Outdated
| "default" | ||
| else | ||
| gemfile | ||
| end |
There was a problem hiding this comment.
This is better but I still prefer:
output_prefix = if gemfile.empty? || gemfile == "Gemfile"
"default"
else
gemfile
endThere was a problem hiding this comment.
@rkrage I reverted my changes and updated rubocop_todo.yml
lib/pg_party/model_injector.rb
Outdated
| @key.first | ||
| else | ||
| @key | ||
| end |
There was a problem hiding this comment.
I reverted my changes and updated rubocop_todo.yml
lib/pg_party/adapter_decorator.rb
Outdated
| "pg_index.indexrelid = pg_class.oid AND relname = #{quote(index_name)}", | ||
| "SCHEMA" | ||
| ).empty? | ||
| select_values(<<-SQL, "SCHEMA").empty? |
There was a problem hiding this comment.
Super tiny nitpick, but I think we should use squiggly heredoc (<<~SQL)
There was a problem hiding this comment.
I followed the style of the previous heredoc in the project. I am also a fan of squiggly heredoc, so I replaced it within the whole project in a separate commit.
igor-alexandrov
force-pushed
the
chore/rubocop
branch
from
64c136f to
9e3118a
Compare
|
@igor-alexandrov, sorry this one fell through the cracks a bit. This project is pretty much in maintenance mode at the moment so I'm not sure how helpful Rubocop will be anymore. Moving to GH actions though is probably a good idea, as I do plan to continue adding support for new Rails versions. All that being said though, if you feel strongly about the Rubocop stuff, feel free to rebase / resolve conflicts and I'll try to get this merged soon. |
igor-alexandrov
force-pushed
the
chore/rubocop
branch
from
f5e1d05 to
8cea11d
Compare
|
@rkrage I rebased the branch. |
2 participants
This PR partially covers #32 issue.
I added RuboCop configuration and GitHub Action (we can go with Circle if you want) to lint the code.
Rules that caused offenses and were fixed (with safe autocorrection):
%w[]notation)Also PR adds autogenerated
.rubocop_todo.ymlwith offenses, that cannot be corrected automatically, they should be discussed individually.Lets discuss.