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

Add option to define index starting integer #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

effektz
Copy link

@effektz effektz commented Dec 22, 2014

No description provided.

@kenn
Copy link
Owner

kenn commented Dec 22, 2014

Can you explain why you need this feature? How and when starting with 0 is not ok? Also, do we really want to accept a start index like 2 or 3, or whatever arbitrary value?

@effektz
Copy link
Author

effektz commented Dec 23, 2014

This is a feature to handle migrations from other Enum gems or other ORMs that aren't ActiveRecord. For example (and my use-case) is migrating from DataMapper to ActiveRecord. In DataMapper, Enum indexes start at 1 instead of 0, and we have a very large production data-set (millions of rows, dozens of columns) that will need to be migrated if we can't tell enum_accessor to offset its index.

@kenn
Copy link
Owner

kenn commented Dec 23, 2014

Thanks for clarifying the real use case.

Based on what you described, I think a global config option would be better than a method option? Otherwise it would look:

enum_accessor :column_a, [:aaa, :bbb], { index_start: 1 }
enum_accessor :column_b, [:ccc, :ddd], { index_start: 1 }

which is not DRY, and having index_start: 1 in one place and index_start: 2 in another is nonsensical.

I also think it should be either 0 or 1, I don't see a point in allowing an arbitrary value other than those two. It's best if we can enforce that in some way.

Therefore, I suggest the following configuration API.

EnumAccessor.config.start_index = :one (or :zero by default)

What do you think?

@effektz
Copy link
Author

effektz commented Dec 31, 2014

@kenn I think that's fair. Although I am of the mind that people should be able to choose any starting index if they want. That being said, I just pushed up changes that creates a configuration option for start_index and locks it to either 0 or 1. Let me know your thoughts.

@effektz
Copy link
Author

effektz commented Feb 24, 2015

@kenn ping - let me know if this looks good to merge or if theres more you'd like to see done.

@kenn
Copy link
Owner

kenn commented Feb 25, 2015

@effektz Thanks! Can you keep the test changes minimal? It's so hard to track changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants