Skip to content

feat: OptimizelyConfigService #188

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 8 commits into from
Jan 7, 2020
Merged

Conversation

oakbani
Copy link
Contributor

@oakbani oakbani commented Dec 26, 2019

Summary

Adds support for OptimizelyConfigService.

Test plan

  • Tested manually with FSC tests for getOptimizelyConfig
  • Added unit tests for OptimizelyConfigService and new entities.

@coveralls
Copy link

Coverage Status

Coverage decreased (-12.3%) to 85.346% when pulling cf62773 on oakbani/optimizely-config into 6a1eec3 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-12.3%) to 85.346% when pulling cf62773 on oakbani/optimizely-config into 6a1eec3 on master.

@coveralls
Copy link

coveralls commented Dec 26, 2019

Coverage Status

Coverage decreased (-0.1%) to 97.479% when pulling b363031 on oakbani/optimizely-config into 6a1eec3 on master.

@zashraf1985
Copy link
Contributor

@aliabbasrizvi It would be great if you can spare some time to review this.

@aliabbasrizvi
Copy link
Contributor

Reviewing this now.

}

$featureKey = $feature->getKey();
$this->featKeyOptlyVariableKeyVariableMap[ $featureKey] = $variablesKeyMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove space in front of $featureKey

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.

LGTM - a tiny format issue

Copy link
Contributor

@aliabbasrizvi aliabbasrizvi 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.

Just rename private variables in all new classes to have leasing underscore.

Good to merge after that.

/**
* @var string Revision of the config.
*/
private $revision;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel there should be a leading underscore in all private variables names

/**
* @var array List of experiments in config.
*/
private $experiments;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same feedback as above

@oakbani
Copy link
Contributor Author

oakbani commented Jan 6, 2020

@aliabbasrizvi Thanks for reviewing. The latest PSR standard for PHP discourages to put a leading underscore before private/protected properties.

Property names MUST NOT be prefixed with a single underscore to indicate protected or private visibility. That is, an underscore prefix explicitly has no meaning.

https://www.php-fig.org/psr/psr-12/

and so did the previous PSR2 standard https://www.php-fig.org/psr/psr-2/

@aliabbasrizvi
Copy link
Contributor

Thanks for pointing that out @oakbani

@aliabbasrizvi
Copy link
Contributor

Going to merge this.

@aliabbasrizvi aliabbasrizvi merged commit cf2c0b3 into master Jan 7, 2020
@aliabbasrizvi aliabbasrizvi deleted the oakbani/optimizely-config branch January 7, 2020 17:28
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.

5 participants