Resolve schemaUrl when merging ResourceInfo #627
Conversation
lalex
requested review from
bobstrecansky,
zsistla,
tidal and
brettmc
as code owners
Codecov Report
@@ Coverage Diff @@
## main #627 +/- ##
============================================
- Coverage 84.45% 84.27% -0.19%
- Complexity 1141 1146 +5
============================================
Files 127 127
Lines 2766 2772 +6
============================================
Hits 2336 2336
- Misses 430 436 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thank you! |
src/SDK/Resource/ResourceInfo.php
Outdated
| @@ -42,15 +45,15 @@ public static function create(AttributesInterface $attributes, ?string $schemaUr | |||
| */ | |||
| public static function merge(ResourceInfo ...$resources): self | |||
There was a problem hiding this comment.
Please note that this method is about to be extracted out.
src/SDK/Resource/ResourceInfo.php
Outdated
| $initialResource = array_shift($resources); | ||
|
|
||
| return array_reduce($resources, function (ResourceInfo $old, ResourceInfo $updating): ResourceInfo { | ||
| return $old->mergeWith($updating); |
There was a problem hiding this comment.
Calling the mergeWith will result in unnecessary object creation when used in defaultResource, however this could be optomized later.
src/SDK/Resource/ResourceInfo.php
Outdated
| @@ -122,4 +125,25 @@ public function getSchemaUrl(): ?string | |||
| { | |||
| return $this->schemaUrl; | |||
| } | |||
|
|
|||
| private function mergeWith(ResourceInfo $resource): ResourceInfo | |||
There was a problem hiding this comment.
Please make this method public and add it to the interface, since static mergeis subject to be moved.
src/SDK/Resource/ResourceInfo.php
Outdated
| $attributes = $this->attributes->toArray() + $resource->getAttributes()->toArray(); | ||
|
|
||
| $schemaUrl = null; | ||
| if (null === $this->schemaUrl) { |
There was a problem hiding this comment.
Please, do not use yoda style.
Please, use the accessor for this property here, since the accessor is used in the updatig resource and this will make it easier to read.
There was a problem hiding this comment.
I'm not sure that php-cs-fixer is configured properly to avoid yoda style.
There was a problem hiding this comment.
No it's not, as there is a lot of it in the code atm, which would needed to be changed (not sure if the fixer can handle this correctly).
However we have agreed in a meetig, that we'd rather want to avoid it in the future.
src/SDK/Resource/ResourceInfo.php
Outdated
| $schemaUrl = null; | ||
| if (null === $this->schemaUrl) { | ||
| $schemaUrl = $resource->getSchemaUrl(); | ||
| } elseif (null === $resource->getSchemaUrl()) { |
src/SDK/Resource/ResourceInfo.php
Outdated
| $schemaUrl = $this->schemaUrl; | ||
| } elseif ($this->schemaUrl === $resource->getSchemaUrl()) { | ||
| $schemaUrl = $this->schemaUrl; | ||
| } else { |
There was a problem hiding this comment.
Please refactor these conditions in a method which simply resolves the schemaURL and remove else/elseif.
There was a problem hiding this comment.
This piece of code is self-describing and follows the specification. A simplified version could hide an original logic.
There was a problem hiding this comment.
I wouldn't go for a simplified version, but rather avoiding the "IFs" with return statements.
I know it's to be found like that in the spec, which doesn't really make the spec better in my eyes.
However avoiding "IFs" is my personal issue, so I'd at least ask for it. Feel free to ignore my request. :)
src/SDK/Resource/ResourceInfo.php
Outdated
| } elseif (null === $resource->getSchemaUrl()) { | ||
| $schemaUrl = $this->schemaUrl; | ||
| } elseif ($this->schemaUrl === $resource->getSchemaUrl()) { | ||
| $schemaUrl = $this->schemaUrl; |
| */ | ||
| public function test_merge_schema_url($oldScheme, $updatingScheme, $expectedScheme, $shouldLog): void | ||
| { | ||
| if ($shouldLog) { |
There was a problem hiding this comment.
Please, do not use conditions in tests. This can simply be a separate test.
|
|
||
| $primary = ResourceInfo::create(new Attributes([]), $oldScheme); | ||
| $secondary = ResourceInfo::create(new Attributes([]), $updatingScheme); | ||
| $result = ResourceInfo::merge($primary, $secondary); |
There was a problem hiding this comment.
Please test against the mergeWithmethod here. (See above)
|
Let's postpone this PR until the |
|
I think you should be able to continue work on this when you'd like to @lalex ! |
|
I've decided to not to go too deep into the process of schema merging. The only significant thing is done is the setting an empty schemaUrl in case of conflict detected. PS. I didn't find a proper way to write a log message from static context. @tidal, could you get a hint? |
Hi @lalex - I don't think logging messages from static methods has come up yet, but looking at |
|
@lalex Thanks for your work!
You could access the LoggerHolder's get method to get the logger as it's static anyway Anyway,I will merge the PR for now. |
Continue with the #445. This PR to resolve a resulting
schemaUrlwhen mergingResourceInfos. Reference to the spec.