unique: true on identifier create_table migration does nothing

[PhilT]

As far as I can tell the unique: true attribute on create_table is not supported. And just as well as the uniqueness is scoped anyway. At least that's what I can understand from a quick glance at the code. Is that right?

[PhilT]

The reason I mention it is that I'm a little unclear on the use of the identifier but the code was telling me conflicting information. So if the above is correct it's basically a version number of the snapshot. And as long as that is unique to the associated model all is good.

[PhilT]

What I can't figure out is why it's needed. It doesn't seem to be used for anything other than to validate it.

[westonganger]

Yes you are correct the identifier only needs to be unique for the parent. In that case the uniqueness constraint should be removed. Feel free to create a PR

[PhilT]

I'd like to create a PR. However, I'm still unclear on the purpose of identifier. As I said it's not really used for anything. Why not just stick with id?

[westonganger]

Developers may not always just want to rollback to the latest version. An identifier allows you to locate any given snapshot much easier.

For example if your currently on "Revision 6" and you want to roll back to "Revision 3". Also consider for example if R1 or R2 does not exist.

[westonganger]

That being said we can probably make it optional. I also desired to change the identifier option from a positional argument to a named argument so this could be a good time to make the change.

Current docs

snapshot = post.create_snapshot!(
  "snapshot_1", # Required
  user: current_user,
  metadata: {
    foo: :bar
  },
)

Updated

snapshot = post.create_snapshot!(
  identifier: "snapshot_1", # recommended, but optional
  user: current_user, # optional
  metadata: { #optional
    foo: :bar
  },
)

[westonganger]

Closing this as the unique constraint issue has been resolved.

Tracking remaining suggestions on #27 and #26