Skip to content

Rails 6.1: change schema dumper condition to only dump explicit default for non identity integers #902

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Apr 28, 2021
Merged

Rails 6.1: change schema dumper condition to only dump explicit default for non identity integers #902

merged 13 commits into from
Apr 28, 2021

Conversation

@mgrunberg
Copy link
Contributor

@mgrunberg mgrunberg commented Apr 21, 2021 •
edited
Loading

PR fixes:

PrimaryKeyAnyTypeTest#test_0001_schema dump primary key includes type and options [/usr/local/bundle/bundler/gems/rails-8b63ea762239/activerecord/test/cases/primary_keys_test.rb:315]:
Expected /create_table "barcodes", primary_key: "code", id: { type: :string, limit: 42 }/ to match "....ActiveRecord::Schema.define(version: 0) do\n\n  create_table \"barcodes\", primary_key: \"code\", id: { type: :string, limit: 42, default: nil }, force: :cascade do |t|\n  end\n\nend\n"

wpolicarpo
wpolicarpo reviewed Apr 21, 2021
View reviewed changes
@mgrunberg mgrunberg mentioned this pull request Apr 21, 2021
Merged
@mgrunberg mgrunberg changed the title Rails 6.1: coerce test to match pk default and binding syntax Rails 6.1: coerce test to match pk default Apr 21, 2021
@mgrunberg
Revert "coerse test to change binding syntax to @n"
bf42b36 
This reverts commit 63eddd4.
@mgrunberg
Merge branch 'main' into issues/yellowspot/rails-6-1/coerce-test-to-m…
c3acfb6 
…atch-pk-default-and-binding-syntax
@mgrunberg
remove test coercion
105c14b
@mgrunberg
set primary_key_nonclustered type to bigint
3b5ba45 
seems this was missing when primary_key changed from integer to bigint
(rails-sqlserver@620baf2#diff-86264ef1995b0800604b26d64ede427e8a3ed59347993bfb409c596e83f4b86f)
@mgrunberg
remove redundat set of primary_key option. AbstractAdapter already do…
d8c570d 
…es this

Absctract SchemaDefinition sets primary_key option
(https://github.com/rails/rails/blob/v6.1.0/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb#L234).

There is no point in doing this
@mgrunberg
remove unnecesary primary check. default_primary_key is called on col…
ba72f0a 
…umns that return true to is_primary?
@mgrunberg
rewrite 'explicit_primary_key_default?'. String primary keys should n…
2943712 
…ot have explicit default
@mgrunberg
specs about primary keys
9c6fee5
@mgrunberg mgrunberg changed the title Rails 6.1: coerce test to match pk default Rails 6.1: change schema dumper condition. Only dump explicit default for non identity integers Apr 23, 2021
@mgrunberg mgrunberg changed the title Rails 6.1: change schema dumper condition. Only dump explicit default for non identity integers Rails 6.1: change schema dumper condition to only dump explicit default for non identity integers Apr 23, 2021
@mgrunberg mgrunberg requested a review from wpolicarpo April 23, 2021 13:13
wpolicarpo
wpolicarpo reviewed Apr 28, 2021
View reviewed changes
Copy link
Member

@wpolicarpo wpolicarpo left a comment

Choose a reason for hiding this comment

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

Can you also rebase your branch?

@mgrunberg mgrunberg requested a review from wpolicarpo April 28, 2021 12:23
@wpolicarpo wpolicarpo merged commit 1cc444b into rails-sqlserver:main Apr 28, 2021
@mgrunberg mgrunberg deleted the issues/yellowspot/rails-6-1/coerce-test-to-match-pk-default-and-binding-syntax branch January 3, 2022 17:31
lavika pushed a commit to lavika/activerecord-sqlserver-adapter that referenced this pull request Sep 26, 2023
@mgrunberg
Rails 6.1: change schema dumper condition to only dump explicit defau…
fc41106 
…lt for non identity integers (rails-sqlserver#902)

* coerse test to change binding syntax to @n

* add default option to pk dump

* Revert "coerse test to change binding syntax to @n"

This reverts commit 63eddd4.

* remove test coercion

* set primary_key_nonclustered type to bigint

seems this was missing when primary_key changed from integer to bigint
(rails-sqlserver@620baf2#diff-86264ef1995b0800604b26d64ede427e8a3ed59347993bfb409c596e83f4b86f)

* remove redundat set of primary_key option. AbstractAdapter already does this

Absctract SchemaDefinition sets primary_key option
(https://github.com/rails/rails/blob/v6.1.0/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb#L234).

There is no point in doing this

* remove unnecesary primary check. default_primary_key is called on columns that return true to is_primary?

* rewrite 'explicit_primary_key_default?'. String primary keys should not have explicit default

* specs about primary keys

* fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpolicarpo wpolicarpo Awaiting requested review from wpolicarpo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mgrunberg @wpolicarpo