Allow symbols as flag names

[Bertg]

About the changes

This PR allows developers to specify flag names as symbols. Ruby developers will often use symbols instead of flag names.

Discussion points

If this PR is not accepted it would be useful to have some sort of clear error at runtime. Today the lib will raise TypeError: no implicit conversion of Symbol into String this is not super useful to the developer.

[gastonfournier]

Hi @Bertg thanks for the contribution, not a Ruby expert, so I'll checkout the code and test myself a bit, but sounds like a good contribution

[coveralls]

Pull Request Test Coverage Report for Build 12414881422

Details

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 95.395%

---

Totals
Change from base Build 12414537848:	0.02%
Covered Lines:	435
Relevant Lines:	456

---

💛 - Coveralls

[Bertg]

@gastonfournier one possible change would be to move this to Unleash.engine / `yggdrasil-engine/ but I'm not sure how that one works.

[gastonfournier]

@gastonfournier one possible change would be to move this to Unleash.engine / `yggdrasil-engine/ but I'm not sure how that one works.

I think it's better how you did it, it's better to convert feature names to string in the edge of the API layer, so internally we only worry about feature as strings. That's also what the other SDKs do, the strongly typed ones only accept strings but in the case of Ruby, calling to_s on the feature seems correct.

[gastonfournier]

An alternative would be to validate that the feature is a string and thrown an error, but that would result in a runtime error with the only difference that the error could be more meaningful. The point being that Unleash API only accepts strings, but this feels like a gray area for languages like ruby where you can't force a type constraint at compile time...

[gastonfournier]

After talking with the team, we're more inclined to keep the API interface as requiring a string. If there's a need of using symbols, the conversion should be done in the surrounding code as other SDKs do. The representation of flags in the application domain, can also be done with objects and to_s doesn't solve that problem. E.g. you could have:

  class MyDomainFeature
    attr_accessor :name, :default_behavior

and in such case you should extract the name from MyDomainFeature. In most cases the mapping from the application domain to the SDK domain should happen in the application logic and we believe the String input of the feature name defines a clear interface with the SDK.

We can keep this open to collect additional feedback, especially since we are in holiday season and some of the main contributors are out these days.

[gastonfournier]

We've talked about it with @sighphyre and we want to also get insights from @rarruda before making a final decision. Just keeping you posted.

[sighphyre]

Hey @Bertg, so I actually quite like this. I think it's consistent with the way we do context fields and generally with most Ruby development.

I have two small concerns:

1. I think blindly calling to_s on arbitrary objects is going to make life really hard when someone eventually passes in something they shouldn't here. I'd like to bound this to only strings and symbols (I realize what we did in the past was also awful but this is a good chance to make this sane)
2. I would like to get @rarruda's inputs on this, I believe he had some concerns

Other than that I think this is worth doing

[Bertg]

OK, I can adjust the PR. But what should we do if we receive something that's is neither a string or symbol?

I have a few suggestions:

• raise RuntimeError, simple clean
• allow it to happen, maybe warn
• Invoke a specific method. Something like .to_feature_name or .to_feature. (I've seen this in other libraries, but I think this is probably out of scope for this).

I would opt for warn now, mark it is deprecated behaviour. Allow the user to switch to a strict mode (raise), which would become the default in a new major version.

But your call, and I'll adjust the PR

[sighphyre]

OK, I can adjust the PR. But what should we do if we receive something that's is neither a string or symbol?

I have a few suggestions:

* raise RuntimeError, simple clean

* allow it to happen, maybe warn

* Invoke a specific method. Something like `.to_feature_name` or `.to_feature`. (I've seen this in other libraries, but I think this is probably out of scope for this).

I would opt for warn now, mark it is deprecated behaviour. Allow the user to switch to a strict mode (raise), which would become the default in a new major version.

But your call, and I'll adjust the PR

I'm tempted to say "let it happen, write something to the error log rather than warn". It's a pretty serious issue in that your flags are definitely not evaluating the way you think they should be but the philisophy of all our SDKs is to default to false rather than fail when and where they can (I'm pretty sure passing goop will result in a false here)