Skip to content
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

feat(decide): add UserContext #248

Merged
merged 15 commits into from
Dec 8, 2020
Merged

feat(decide): add UserContext #248

merged 15 commits into from
Dec 8, 2020

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Nov 9, 2020

Summary

A part of multiple PRs for Decide API

  • added OptimizelyUserContext
  • added CreateUserContext API

Tests

  • Unit tests added.

BugFix of userAttributes
Unit tests fix
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

please address.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

some more comments.

mnoman09 and others added 4 commits November 13, 2020 20:41
Throwing not implemented exceptions for not implemented functions in optimizelyUserContext
Added detailed documentation of optimizelyUserContext and Decision class
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Looks good! Need some changes.

@mnoman09 mnoman09 requested a review from jaeopt November 26, 2020 09:04
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

All changes look good. I see mutex control for reading attributes is missing.


public static IDecisionReasons NewInstance(List<OptimizelyDecideOption> options)
{
if (options != null && options.Contains(OptimizelyDecideOption.INCLUDE_REASONS))
Copy link
Contributor

Choose a reason for hiding this comment

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

options?.Contains() ?? false ? new DefaultDecisionReasons() : new ErrorsDecisionReasons

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

please address.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

Apporved with a comment:

  • Thread-safe attributes read/write will be fixed in a separate PR (OASIS-7371)

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm

@jaeopt jaeopt merged commit 3031bd5 into master Dec 8, 2020
@jaeopt jaeopt deleted the mnoman/user-context branch December 8, 2020 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants