Skip to content

Alda/numeric metrics #76

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Sep 13, 2017
Merged

Alda/numeric metrics #76

merged 20 commits into from
Sep 13, 2017

Conversation

alda-optimizely
Copy link
Contributor

No description provided.

… event value to revenue value to better describe its reference.
…eration are valid when only revenue or numeric metric event tags are provided.
…stic way, which made some unit tests inconsistent. Event tag keys is a short list, so the sort should not create overhead.
… event object checks to parameters related to event tags so that the unit tests won't be brittle.
…mbers are supported; everything else should return None.
@optibot
Copy link

optibot commented Sep 13, 2017

Can one of the admins verify this patch?

@coveralls
Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage increased (+0.1%) to 96.667% when pulling 57200c9 on alda/numeric-metrics into 70e1d44 on master.

@mikeproeng37
Copy link
Contributor

build

@mikeproeng37
Copy link
Contributor

build

@coveralls
Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage increased (+0.1%) to 96.667% when pulling ccad939 on alda/numeric-metrics into 70e1d44 on master.

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alda-optimizely alda-optimizely merged commit 788d66c into master Sep 13, 2017
alda-optimizely added a commit that referenced this pull request Oct 6, 2017
* Added a helper method to find numeric metric values in the event tags.

* Added tests for the numeric metric value helper method.

* Renamed REVENUE_METRIC_TYPE enum to EVENT_VALUE_METRIC to be consistent with Java.

* Replaced hard-coded revenue tag with enum. Renamed the local variable event value to revenue value to better describe its reference.

* Added a check for numeric metric value in the event builder.

* Fixed unit tests for numeric metric.

* Changed event value test names to event tags. Updated test comment descriptions.

* Added a couple more unit tests to test track and conversion event generation are valid when only revenue or numeric metric event tags are provided.

* Fixed lint errors.

* Enabled max diff to see unit test error on travis.

* Fixed nit spacing and wording.

* Sort the event tag keys as they were being returned in an undeterministic way, which made some unit tests inconsistent. Event tag keys is a short list, so the sort should not create overhead.

* Refactored numeric metric unit tests to use a subroutine and isolated event object checks to parameters related to event tags so that the unit tests won't be brittle.

* Handled corner cases for the get numeric value better. Strings and numbers are supported; everything else should return None.

* Sorted the event features results in the test so we don't have to sort it in the event builder.

* Fixed an import statement.

* Removed basestring as this is not supported by Python3.

* Updated some code comments.

* Cleaned up some wording and unit test ordering.

* Removed blank line at end of file.
@aliabbasrizvi aliabbasrizvi deleted the alda/numeric-metrics branch October 9, 2017 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants