search

mint-metrics / mojito-js-delivery

🧪 Source-controlled JS split testing framework for building and launching A/B tests.
https://mojito.mx/docs/js-delivery-intro
Other
16 stars 29 forks source link

Cleanup mojito #37

Closed dapperdrop closed 4 years ago

dapperdrop commented 4 years ago

Ref: https://github.com/mint-metrics/mojito-js-delivery/issues/35

dapperdrop commented 4 years ago

Maybe make this comment a bit clearer to reflect the logic:

"Assign recipes to users in test, else exclude them by test's sample rate"

Done: https://github.com/mint-metrics/mojito-js-delivery/pull/37/commits/062d8e384db25e54802e8820edfee54eaf59daf8

allmywant commented 4 years ago

@dapperdrop

if (!recipe.js) { recipe.js = function () {}; } Then I will remove it in another commit. This should be the last item to resolve for this PR, unless you guys see something else should be addressed.

Yes, we don't need it. I also left some comments in this pr but it seems not visible to you guys. e.g. :

Move var seededRNGSeed = (new Date()).getTime(); here means the initial seed will be created every time we call getSeededRNGRandom function, seems not correct. This the only question I'd like to confirm before approving.

Please let me know if you have seen it, maybe I did something wrong when I submit review comments.

dapperdrop commented 4 years ago

Thanks @allmywant - I didn't see your other comments, thanks for letting us know.

Move var seededRNGSeed = (new Date()).getTime(); here means the initial seed will be created every time we call getSeededRNGRandom function, seems not correct. This the only question I'd like to confirm before approving.

Good spot there. I'm not sure either, @kingo55 do you know?

kingo55 commented 4 years ago

@allmywant - did you "Submit" the code review? I had that issue the other day where I thought I added questions to a PR, but they weren't visible until I hit the "Submit review" section. Github code review submission is not so clear IMO.

Screen Shot 2019-12-20 at 10 59 37
kingo55 commented 4 years ago

Move var seededRNGSeed = (new Date()).getTime(); here means the initial seed will be created every time we call getSeededRNGRandom function, seems not correct. This the only question I'd like to confirm before approving.

Good spot there. I'm not sure either, @kingo55 do you know?

I'm not sure. Perhaps it's safer for us to move it back so we're not unintentionally changing the logic of this old function at all.