Conversation
1 similar comment
|
@aliabbasrizvi It would be great if you can spare some time to review this. |
|
Reviewing this now. |
| } | ||
|
|
||
| $featureKey = $feature->getKey(); | ||
| $this->featKeyOptlyVariableKeyVariableMap[ $featureKey] = $variablesKeyMap; |
There was a problem hiding this comment.
Remove space in front of $featureKey
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
LGTM - a tiny format issue
aliabbasrizvi
left a comment
aliabbasrizvi
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
I feel there should be a leading underscore in all private variables names
| /** | ||
| * @var array List of experiments in config. | ||
| */ | ||
| private $experiments; |
There was a problem hiding this comment.
Same feedback as above
|
@aliabbasrizvi Thanks for reviewing. The latest PSR standard for PHP discourages to put a leading underscore before private/protected properties.
https://www.php-fig.org/psr/psr-12/ and so did the previous PSR2 standard https://www.php-fig.org/psr/psr-2/ |
|
Thanks for pointing that out @oakbani |
|
Going to merge this. |
5 participants
Summary
Adds support for OptimizelyConfigService.
Test plan