variants in strategies #148

gardleopard · 2023-08-22T06:41:42Z

Description
Implementation of Strategy Variants

Type of change
Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
How Has This Been Tested?
By running specifications locally

Just ready for early review

coveralls · 2023-08-22T10:33:08Z

Pull Request Test Coverage Report for Build 5961861904

85 of 85 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.06%) to 97.011%

Totals
Change from base Build 5463810032:	0.06%
Covered Lines:	2499
Relevant Lines:	2576

💛 - Coveralls

lib/unleash/feature_toggle.rb

lib/unleash/variant_definition.rb

lib/unleash/feature_toggle.rb

rarruda

Note: not linted any of my examples, or ran QA on all suggestions.

But i think it was mostly these things that I noticed at this time:

would like a few more tests specially for feature_toggle
prefer to keep original tests untouched, unless required.
move parsing logic for variant_definitions up the stack.
extract evaluation_result to it's own FeatureEvaluationResult class
ruby idiomatic: is_enabled -> enabled possibly even add enabled? method
lots of bike shedding (variants -> variant_definitions)
need of == method / use of Comparable in VariantDefinition
tiny issues in debugging message within evaluate()

spec/unleash/activation_strategy_spec.rb

lib/unleash/activation_strategy.rb

lib/unleash/variant_definition.rb

lib/unleash/feature_toggle.rb

lib/unleash/activation_strategy.rb

lib/unleash/feature_toggle.rb

spec/unleash/feature_toggle_spec.rb

gardleopard · 2023-08-23T07:52:24Z

@rarruda ready for round two imho:)

… specs

rarruda · 2023-08-23T09:15:37Z

I can take a look a second look at the PR this evening!

gardleopard · 2023-08-23T09:22:34Z

Super, then ill check it tomorrow @rarruda :)

rarruda · 2023-08-23T20:46:41Z

lib/unleash/feature_toggle.rb

+
+    def evaluate(context)
+      evaluation_result = {
+        is_enabled: false,


Actually the most ruby idiomatic would be:

:enabled?

But if you don't like that suggestion maybe consider:

:is_enabled?

Either way would be nice to have your opinion here, even if it is that you intend to keep it as is.

rarruda · 2023-08-23T20:48:05Z

lib/unleash/feature_toggle.rb

+      evaluation_result = {
+        is_enabled: false,
+        strategy: nil
+      }


I would like to re-iterate my recommendation for using an object here in the second review. Even a Struct would do the job. Passing around fixed hashes feels dirty/hacky.

rarruda · 2023-08-23T20:49:06Z

lib/unleash/feature_toggle.rb


      Unleash.logger.debug "Unleash::FeatureToggle (enabled:#{self.enabled} " \
-        "and Strategies combined with contraints returned #{result})"
+        "and Strategies combined with contraints returned #{evaluation_result[:result]})"


This bug is still pending resolution.

rarruda · 2023-08-23T20:49:29Z

lib/unleash/feature_toggle.rb

-      variant_weight = Unleash::Strategy::Util.get_normalized_number(variant_salt(context, stickiness), self.name, sum_variant_defs_weights)
+    def variant_from_weights(context, stickiness, variants, group_id)
+      variant_weight = Unleash::Strategy::Util.get_normalized_number(
+        variant_salt(context, stickiness), group_id,


I still think it would make it for more readable code if you added a new line here.

rarruda · 2023-08-23T20:56:20Z

lib/unleash/feature_toggle.rb

+      if self.strategies.empty?
+        evaluation_result[:is_enabled] = true
+      else
+        evaluation_result[:strategy] = self.strategies.find(proc{ nil }) do |s|


You didn't address this yet.

rarruda · 2023-08-23T21:22:16Z

lib/unleash/feature_toggle.rb

+      variant_definitions = evaluation_result[:strategy]&.variant_definitions || self.variant_definitions
+      variant_definitions = self.variant_definitions if variant_definitions.empty?


nit pick/bike shedding for legibility:

(So there is only one override of values, with all the conditions clearly outlined in one line)

Suggested change

variant_definitions = evaluation_result[:strategy]&.variant_definitions || self.variant_definitions

variant_definitions = self.variant_definitions if variant_definitions.empty?

variant_definitions = evaluation_result[:strategy]&.variant_definitions

variant_definitions = self.variant_definitions if variant_definitions.nil? || variant_definitions.empty?

lib/unleash/variant_definition.rb

rarruda

I think it was mostly these things that I noticed at this time:

ruby idiomatic: is_enabled -> enabled possibly even enabled?
== method is still present
debugging message within evaluate() still contains a bug
simplify by converting.fetch -> .to_h.[] ||
legibility improvement :self.strategies.find(proc{ nil }) do -> self.strategies.find do.
legibility improvement : only one override of values when possible
legibility improvement : line break
extract evaluation_result to it's own FeatureEvaluationResult class
simplify logic in evaluate() by making the three scenarios explicit via if/elsif/else

Some where just not addressed from the previous review.

But I think things will look really great if you can address the things i mentioned. 😄

lib/unleash/variant_definition.rb

lib/unleash/feature_toggle.rb

gardleopard · 2023-08-24T08:47:58Z

resolves #147

rarruda · 2023-08-24T09:10:28Z

Bikeshedding, det handler om styl. Og er ikke viktig. Men siden man bruke Struct for FeatureEvaluationResult, hadde kanskje ble penere å behandle den som objekt og bruke access metoder, istede av å hente verdi via [:enabled]?

lib/unleash/feature_toggle.rb

variants in strategies passes tests

617dacc

Just ready for early review

gardleopard marked this pull request as ready for review August 22, 2023 10:30

sjaanus reviewed Aug 22, 2023

View reviewed changes

lib/unleash/feature_toggle.rb Outdated Show resolved Hide resolved

gardleopard requested a review from rarruda August 22, 2023 10:54

gardleopard changed the title ~~variants in strategies passes tests~~ variants in strategies Aug 22, 2023

rarruda reviewed Aug 22, 2023

View reviewed changes

lib/unleash/variant_definition.rb Outdated Show resolved Hide resolved

gardleopard force-pushed the new_client_spec branch from cfa7a92 to 53aadf8 Compare August 22, 2023 11:12

refactoring

78c1e48

gardleopard force-pushed the new_client_spec branch from 53aadf8 to 78c1e48 Compare August 22, 2023 11:14

rubocop

ccbd524

kwasniew reviewed Aug 22, 2023

View reviewed changes

lib/unleash/feature_toggle.rb Outdated Show resolved Hide resolved

gardleopard added 2 commits August 22, 2023 13:30

refactor: make method name more clear and return object better

30a3dbb

refactor: make language and objects more clear

b8bfff8

kwasniew approved these changes Aug 22, 2023

View reviewed changes

rarruda reviewed Aug 22, 2023

View reviewed changes

gardleopard added 2 commits August 23, 2023 09:37

refactor: change how we initialize variants

4870833

refactor: s/variant/variant_definition/g

8d3d682

gardleopard added 4 commits August 23, 2023 10:31

test: add test inside project for ppl that are not running the client…

fb48ba4

… specs

fix: bug on filtering fixing constraints and variant definitions

b219c37

rubocop

f724e05

new test for new parameter

c1ccd29

rarruda reviewed Aug 23, 2023

View reviewed changes

gardleopard added 3 commits August 24, 2023 08:43

refactor - phase 1

447c43b

refactor round 2

a6d3ea3

refactor phase 3

85f025c

rarruda approved these changes Aug 24, 2023

View reviewed changes

lib/unleash/feature_toggle.rb Outdated Show resolved Hide resolved

print complete object in debug

437c965

gardleopard merged commit a3eabdf into main Aug 24, 2023
44 checks passed

gardleopard deleted the new_client_spec branch August 24, 2023 09:38

gardleopard mentioned this pull request Aug 24, 2023

Strategy variants #147

Closed

rarruda mentioned this pull request Sep 12, 2023

refactor: FeatureEvaluationResult struct as object #151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

variants in strategies #148

variants in strategies #148

gardleopard commented Aug 22, 2023 •

edited

Loading

coveralls commented Aug 22, 2023 •

edited

Loading

rarruda left a comment

gardleopard commented Aug 23, 2023

rarruda commented Aug 23, 2023

gardleopard commented Aug 23, 2023

rarruda Aug 23, 2023 •

edited

Loading

rarruda Aug 23, 2023

rarruda Aug 23, 2023

rarruda Aug 23, 2023

rarruda Aug 23, 2023

rarruda Aug 23, 2023 •

edited

Loading

rarruda left a comment •

edited

Loading

gardleopard commented Aug 24, 2023

rarruda commented Aug 24, 2023

		variant_definitions = evaluation_result[:strategy]&.variant_definitions \|\| self.variant_definitions
		variant_definitions = self.variant_definitions if variant_definitions.empty?

variants in strategies #148

variants in strategies #148

Conversation

gardleopard commented Aug 22, 2023 • edited Loading

coveralls commented Aug 22, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5961861904

💛 - Coveralls

rarruda left a comment

Choose a reason for hiding this comment

gardleopard commented Aug 23, 2023

rarruda commented Aug 23, 2023

gardleopard commented Aug 23, 2023

rarruda Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

rarruda Aug 23, 2023

Choose a reason for hiding this comment

rarruda Aug 23, 2023

Choose a reason for hiding this comment

rarruda Aug 23, 2023

Choose a reason for hiding this comment

rarruda Aug 23, 2023

Choose a reason for hiding this comment

rarruda Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

rarruda left a comment • edited Loading

Choose a reason for hiding this comment

gardleopard commented Aug 24, 2023

rarruda commented Aug 24, 2023

gardleopard commented Aug 22, 2023 •

edited

Loading

coveralls commented Aug 22, 2023 •

edited

Loading

rarruda Aug 23, 2023 •

edited

Loading

rarruda Aug 23, 2023 •

edited

Loading

rarruda left a comment •

edited

Loading