Skip to content
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.

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

Make the primary key to use for the migration configurable #73

Closed
wants to merge 3 commits into from

Conversation

ags
Copy link

@ags ags commented Oct 1, 2014

This a squashed and rebased version of #49, since @airhorns seems unreachable.
There were a few conflicts, which I've resolved. It'd be great if we can get this looked at again!


#49

Instead of relying on a table to have a primary key, we now accept the
order_column option and put that everywhere. This kind of uncomfortable
state about the migration now needs to be given to everyone and I think
an appropriate place is the migration, since most objects in the system
have access to it and they all need to agree on the value. This required
passing the options down a couple more objects for the Migrator creating
the Migration to have access, but I think thats acceptable for this
increase in functionality.

@iamliamnorton
Copy link

Nice work @ags, hopefully this gets merged soon.

@ags ags changed the title Allow specification of the order_column to remove need for a primary key Make the primary key to use for the migration configurable Oct 2, 2014
@arthurnn
Copy link
Contributor

@ags thanks for the PR, do you mind rebasing this against latest master and add a changelog entry for it?
thanks

@ags ags force-pushed the configurable_primary_key branch 3 times, most recently from 23851b8 to bcae636 Compare March 25, 2015 11:57
ags added a commit to ags/lhm that referenced this pull request Mar 25, 2015
@ags ags force-pushed the configurable_primary_key branch from bcae636 to 91a6468 Compare March 25, 2015 12:02
ags added a commit to ags/lhm that referenced this pull request Mar 25, 2015
@ags ags force-pushed the configurable_primary_key branch from 91a6468 to d373e71 Compare March 25, 2015 12:09
ags added a commit to ags/lhm that referenced this pull request Mar 25, 2015
@ags ags force-pushed the configurable_primary_key branch 4 times, most recently from 633ec01 to be8004d Compare March 25, 2015 13:14
ags added a commit to ags/lhm that referenced this pull request Mar 25, 2015
@ags ags force-pushed the configurable_primary_key branch from be8004d to 4b235e4 Compare March 25, 2015 21:53
ags added a commit to ags/lhm that referenced this pull request Mar 25, 2015
@ags ags force-pushed the configurable_primary_key branch from 4b235e4 to 288a4a5 Compare March 25, 2015 22:00
ags added a commit to ags/lhm that referenced this pull request Mar 25, 2015
@ags ags force-pushed the configurable_primary_key branch from 288a4a5 to efb5e37 Compare March 25, 2015 22:03
ags added a commit to ags/lhm that referenced this pull request Mar 25, 2015
airhorns and others added 3 commits March 26, 2015 09:08
Instead of relying on a table to have a primary key, we now accept the
order_column option and put that everywhere. This kind of uncomfortable
state about the migration now needs to be given to everyone and I think
an appropriate place is the migration, since most objects in the system
have access to it and they all need to agree on the value. This required
passing the options down a couple more objects for the Migrator creating
the Migration to have access, but I think thats acceptable for this
increase in functionality.
@ags ags force-pushed the configurable_primary_key branch from efb5e37 to bc819f2 Compare March 25, 2015 22:08
@mockra
Copy link

mockra commented Nov 6, 2015

Sorry for the bump, but any idea if/when this will be merged? Thanks!

@arthurnn arthurnn self-assigned this Nov 7, 2015
@@ -188,7 +189,11 @@ def validate
end

unless @origin.satisfies_id_autoincrement_requirement?
Copy link

Choose a reason for hiding this comment

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

There's no way for this to not error out when an order_column is specified.

@origin.satisfies_id_autoincrement_requirement? checks the id column, even if an order_column is set. Once that check fails, an error is set no matter what. The code should probably look more like:

      if @options[:order_column]
        if !@origin.can_use_order_column?(@options[:order_column])
          error("origin does not satisfy primary key requirements")
        end
      else
        if !@origin.satisfies_id_autoincrement_requirement?
          error("origin does not satisfy primary key requirements")
        end
      end

Sorry for the comment, not sure what the best way to submit changes to a PR is. Thanks!

@ags ags closed this Feb 1, 2017
@ags ags unassigned arthurnn Feb 1, 2017
@ags ags deleted the configurable_primary_key branch February 1, 2017 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants