Add support for Postgres' minimum_value, maximum_value, and cycle properties#23
Add support for Postgres' minimum_value, maximum_value, and cycle properties#23NuckChorris wants to merge 2 commits intofnando:mainfrom
Conversation
The test helpers are in test/test_helper.rb
NuckChorris
left a comment
NuckChorris
left a comment
There was a problem hiding this comment.
Left some comments to explain my thinking in more detail 😄
| options = [] | ||
|
|
||
| if start_value && Integer(start_value) != 1 | ||
| if start_value && Integer(start_value) != Integer(minimum_value) |
There was a problem hiding this comment.
This corrects the logic to match the postgres documentation. The default start_value is actually minimum_value, which defaults to 1. Now that you can change minimum_value to things other than 1, the logic has to change accordingly.
| options << "min: #{minimum_value}" | ||
| end | ||
|
|
||
| if maximum_value && Integer(maximum_value) != (2**63 - 1) |
There was a problem hiding this comment.
Technically this maximum value only applies to ascending (default) sequences with bigint (default) type, so if we want to add support for descending sequences or non-bigint sequences, we would need to correct this logic.
Presumably we could use information_schema.sequences.data_type to check the max (or do some math based on information_schema.sequences.numeric_precision maybe?) and check that increment is negative to detect descending sequences, but I'm not even sure why you would use either of those features, and they would be a separate PR regardless.
| # frozen_string_literal: true | ||
|
|
||
| module TestHelper | ||
| def recreate_table | ||
| ActiveRecord::Schema.define(version: 0) do | ||
| begin | ||
| drop_table(:things) | ||
| rescue StandardError | ||
| nil | ||
| end | ||
| execute "drop sequence if exists position" | ||
| execute "drop sequence if exists a" | ||
| execute "drop sequence if exists b" | ||
| execute "drop sequence if exists c" | ||
| execute "drop sequence if exists d" | ||
| execute "drop sequence if exists my_schema.e" | ||
|
|
||
| create_table :things do |t| | ||
| t.integer :quantity, default: 0 | ||
| t.string :slug | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def with_migration(&block) | ||
| migration_class = if ActiveRecord::Migration.respond_to?(:[]) | ||
| ActiveRecord::Migration[ | ||
| ActiveRecord::Migration.current_version | ||
| ] | ||
| else | ||
| ActiveRecord::Migration | ||
| end | ||
|
|
||
| Class.new(migration_class, &block).new | ||
| end | ||
| end |
There was a problem hiding this comment.
This code duplicates the helpers in test/test_helper.rb but slightly different and completely unused, so I deleted it to not confuse future developers (I added a change here but then it didn't work!)
It’s a fork for now, working to upstream in fnando/ar-sequence#23
It’s a fork for now, working to upstream in fnando/ar-sequence#23
It’s a fork for now, working to upstream in fnando/ar-sequence#23
It’s a fork for now, working to upstream in fnando/ar-sequence#23
1 participant
PR Checklist
PR Structure
PRs).
two PRs otherwise).
Thoroughness
fixes.
.mdfiles, etc… affected by this change.What
Adds support for Postgres'
minimum_value,maximum_value, andcycleproperties, with full test coverage and regurgitation intoschema.rbAlso quickly cleared out a dead file that tripped me up while I was writing the tests, hope you don't mind that (but I'm open to making that a separate PR if you'd prefer)
Why
I needed a cycling postgres sequence for a function I was writing to generate snowflake IDs. Cycling obviously doesn't make much sense if you can't define a range for it. Thus, the trio of new, fully-optional parameters.
Known limitations
N/A