Support v0.4.0 spec #11
Conversation
Based on this growthbook/growthbook#959 and the related docs to build your own SDK https://docs.growthbook.io/lib/build-your-own.
| @@ -3270,7 +3270,7 @@ | |||
| "hashVersion": 2, | |||
| "variations": [0, 1], | |||
| "weights": [0.5, 0.5], | |||
| "coverage": [0.99] | |||
| "coverage": 0.99 | |||
There was a problem hiding this comment.
Is this a legit fix or should the SDK be ready to receive an array here? 🤔
There was a problem hiding this comment.
Coverage should be a float. There was an error in the initial cases.json file where it was specified as an array of floats.
| return (FNV.new.fnv1a_32(value + seed) % 1000) / 1000.0 if version == 1 | ||
| return (FNV.new.fnv1a_32(FNV.new.fnv1a_32(seed + value).to_s) % 10000) / 10000.0 if version == 2 | ||
|
|
||
| -1 |
There was a problem hiding this comment.
Based on the updated 0.4.1 tests, this method should return nil. We are doing a less than check which returns truthy for -1 when it shouldn't, and this was fixed in 0.4.1.
| @@ -148,5 +151,9 @@ def self.get_query_string_override(id, url, numVariations) | |||
|
|
|||
| n | |||
| end | |||
|
|
|||
| def self.in_range(n, range) | |||
There was a problem hiding this comment.
My thoughts are this may be better as def self.in_range?(n, range)
| 'name' => @name, | ||
| 'phase' => @phase, | ||
| 'tracks' => @tracks | ||
| }.compact |
There was a problem hiding this comment.
Nice 👍 Is this tested in new or existing test cases so we can make sure it still returns the same output?
There was a problem hiding this comment.
Thanks for this! 🚀
I left some minor feedback but nothing blocking. I will merge this for now and you are welcome to do 0.4.1 or we can do it, just let us know if you'd like to do it.
With changes to 0.4.1 it adds support for decrypting encrypted features, which would address this new open ticket: #12
|
Hey @tinahollygb!
I don't think I'll have time in the next couple of weeks. Hope you can do it meanwhile 😸 Also, are you planning to publish a new release including the recent changes? Something less urgent or important, are you interested in any of these possible improvements in the future?
|
|
Thanks @dsantosmerino, we can take care of the 0.4.1 changes.
I will discuss with the rest of the team when the next publish is appropriate, but I know we have a bug fix in 0.4.1 that impacts the new hashing, so I'm hesitant to publish a new version before that gets out, though it's unlikely that bug would impact anyone so maybe it's ok to still publish, I'll check with others. This was fixed last week in the JS SDK if I'm not mistaken, so you may not be aware of it (we now return null instead of -1 when an invalid hash version is provided). We also have plans to add decrypting of encrypted payloads, which is required to support version 0.4.1 of the test case file. I will chat with the team to figure out when the right time is to publish a new release, i.e. if we should do it at 0.4.1 support or if 0.4.0 is ok.
Yes, very interested! I even got started on it yesterday. You can review the PR if you like: #14
The PR covers adding Rubocop and CI processes that include testing on 2.5 and 3.2.2. I did not add jruby or some of the other ones but feel free to review and offer suggestions if you think we should cover more. I have personally never used jruby, and I have not heard of some of the others that the Github Action supports. Let me know if you have any other thoughts. I do know for sure that we cannot support 2.4 due to syntax. In that PR, there are failing builds that can be seen when checking coverage for lower versions. As for the unused classes, I'm going to address that now in this PR as it was suggested to remove those. I'll have to chat with another teammate to see what can be deleted as many of these are still used. In the meantime, if you have some bandwidth, please review the changes in the PR that addresses the improvement suggestions: #14 Thanks! |
Support v0.4.0 spec, based on this growthbook/growthbook#959 and the related docs to build your own SDK https://docs.growthbook.io/lib/build-your-own. These docs and the language agnostic
cases.jsonfile are really useful to maintain the SDKs up to date, congrats 👏While I attempted to only modify the required classes and methods to match the new specification, I believe there is room for improving the SDK. Specifically, I suggest:
User,Experiment,ExperimentResult, and some methods seem to no longer be required and can be cleaned up.