allow initialization with Hash

[kshnurov]

@kenn thank you for this beautiful and elegant gem! I've added an ability to initialize it with Hash. It is a safer & better option:

• you won't be able to accidentally break everything by deleting or moving one of the keys
• you can specify keys in any order and use any integer values (as long as they have a base of 2)
• you can safely remove any keys in case you don't need them or don't want their methods to be available

Also, added a validation for duplicate keys when initializing with an Array.

[kshnurov]

Added: simplified Value class, it's useless without definition & instance, so they should be passed during initialization. Also got rid of method_missing

[kshnurov]

@kenn hi! Have you had a chance to look into this?

[kenn]

Thanks for the PR @kshnurov,

I agree with the benefits of having those features at a high level, but I'm not sure if just exposing the internal structure (which has a lot more surface areas) is the best way to do it.

Namely, the API:

flag :languages, { english: 1, spanish: 2, chinese: 4, french: 8, japanese: 16 }

does not look good to me.

Why not take numbers as a bit position like { english: 1, spanish: 2, chinese: 3, french: 4, japanese: 5 } and let the library calculate the base of two? Or why not take [:english, :_, :chinese, :_, :_] for deleted keys? When we allow deleted keys, do we actually want to fill them with zeros? (then you have to remember to do it BEFORE you delete it from the codebase)

Possible combinations of potholes / unknown unknowns would explode per each surface area added, so we should be really careful if we are addressing real problems that pains a lot.

So tell me real stories what went wrong and how bad it was. That conversation would guide us what would be the ideal solutions.

[kshnurov]

Hi @kenn Using positions might sound more convenient in simple use cases, but these are the reasons I'm using values instead of positions:

1. It's simple, transparent and obvious. What you put in is what you get stored. For code readability you can still use 2**3, 2**4, 2**5, ...
2. It is consistent with what .maps and .raw will return. Setting { english: 5 } and getting .maps = { english: 32 } is confusing
3. It leads to fewer errors. Even your example contains one - bit positions start from 0. How many people will start from 1 and not use the 1st bit? And accidentally putting values instead of positions will quickly overflow int64.
4. You can use whatever variables or bitmasks you have to set values without having to do Math.log(x,2) on each one, which will be a nightmare

As for deleted keys - first of all, you can't do [:english, :_, :chinese, :_, :_], we're using Set underneath, so every value should be unique, having 2+ identical keys will completely break the behavior. That's why I've added extra validation for that. The whole reason for deleted keys is that they stay in the DB in case you need them later (otherwise you should nullify them by yourself and just remove the key, no need for library here), but they are completely gone from .maps and .to_a, which are used for output, you can't .set them, and etc.

[kenn]

Using positions might sound more convenient in simple use cases, but these are the reasons I'm using values instead of positions:

1. I'm pretty sure most people prefer 2**38 rather than 274877906944 😉
2. Fair point
3. Disagree — exactly why I'm reluctant to accept this PR — these new problems / confusions don't exist right now
4. I don't get — in what cases you need to do it?

As for deleted keys - first of all, you can't do [:english, :_, :chinese, :_, :_], we're using Set underneath, so every value should be unique, having 2+ identical keys will completely break the behavior. That's why I've added extra validation for that.

I'm only talking about the API.

We could give a unique key to each :_ underneath, like :_1, :_2 automatically, but it's an implementation detail that can be changed as we want later on. Exposed APIs are hard to change, however. That's why I'm leaning on a very, very simple API.

The whole reason for deleted keys is that they stay in the DB in case you need them later (otherwise you should nullify them by yourself and just remove the key, no need for library here), but they are completely gone from .maps and .to_a, which are used for output, you can't .set them, and etc.

Then, what's the case of "need them later" like? Will you describe a scenario that mandates this feature? I'm imagining most of those could be solved without needing code edits.

I always welcome real world problems rather than a solution waiting for a problem.

[kshnurov]

3. Disagree — exactly why I'm reluctant to accept this PR — these new problems / confusions don't exist right now

The old format also has its own problems, and it isn't going anywhere, you can still use both - simple one where you can't move/delete keys or specify them in any order, or a more advanced one, where you have to be careful with the definition, but it gives you more freedom and control.

4. I don't get — in what cases you need to do it?

This isn't some extra code/feature added here, it's just a free bonus of this format, so I don't think it makes sense to fantasize if someone needs it or not.

That's why I'm leaning on a very, very simple API.

Me too, that's why I'm against any workarounds like _. You don't need a key - you just remove it from the definition, that's it. No extra code inside the gem, no extra code to filter out _ from .to_a on the user end. This is not possible with the old format, which is still there, you can use it if you want.

Then, what's the case of "need them later" like? Will you describe a scenario that mandates this feature? I'm imagining most of those could be solved without needing code edits.

E.g. completely disable some feature for a while (until it's fixed, production-ready or whatever), but keep the data whoever has it on or off. With the old format you'll have to comment/condition all the code of this feature, with the new one all you need to do is remove one key from the hash, takes 2 seconds.

[kshnurov]

@kenn is there anything left that stops you from merging this?

[kenn]

E.g. completely disable some feature for a while (until it's fixed, production-ready or whatever), but keep the data whoever has it on or off. With the old format you'll have to comment/condition all the code of this feature, with the new one all you need to do is remove one key from the hash, takes 2 seconds.

Hmm, I don't quite get it. If you are temporarily disabling a feature with an intention to bring it back later on, wouldn't you just leave the data AND code as it is (because you'll need them later), and do more abstraction elsewhere?

Here's a fake case (I very much appreciate more realistic cases, though):

class User
  flag :notifications, [:email, :push, :sms, :in_app]
end

user.notifications.sms?

Now say you want to temporarily disable SMS because the API vendor has gone out of business and alternatives are too expensive. You would still need to edit and/or delete user.notifications.sms? from everywhere in the code base, right? Otherwise it would raise an exception. But I don't want to delete the code and re-implement it, so I'd just make changes so that it does nothing, rather than messing with the flag definitions. I'd be happy to leave the definition as is.

Now I'm more leaning against adding this feature unless you give me a better and more convincing use case scenario.

[kshnurov]

You would still need to edit and/or delete user.notifications.sms? from everywhere in the code base, right? Otherwise it would raise an exception.

Not if you're using user.notifications.set?(:sms), which you should, it'll just return false.

Now I'm more leaning against adding this feature unless you give me a better and more convincing use case scenario.

I already gave you a lot of reasons why this should be added to avoid major problems in the future. If you still don't understand that - just close this PR. I don't want to waste any more time on this, considering some poor code also fixed with this PR, it'll be easier to create another gem without such design flaws than to convince you.