-
-
Notifications
You must be signed in to change notification settings - Fork 27
Convert envelope trait to abstract class #231
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
base: master
Are you sure you want to change the base?
Conversation
@@ -37,12 +37,4 @@ public function getMetadata(): array | |||
{ | |||
return $this->metadata; | |||
} | |||
|
|||
public function withMetadata(array $metadata): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any real-world use cases. Do you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message handlers may add metadata into the message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should wrap it into an envelope then.
src/Message/Envelope.php
Outdated
@@ -23,14 +20,6 @@ public function getMessage(): MessageInterface | |||
return $this->message; | |||
} | |||
|
|||
public function withMessage(MessageInterface $message): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any real-world use cases. Do you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question.
Why is this better than FooEnvelope::fromMessage($message)
or new FooEnvelope()
?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #231 +/- ##
============================================
+ Coverage 96.16% 96.47% +0.30%
+ Complexity 359 356 -3
============================================
Files 47 47
Lines 966 965 -1
============================================
+ Hits 929 931 +2
+ Misses 37 34 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems, this PR significantly degraded performance (see benchmark CI).
The performance concern is valid. |
@@ -37,12 +37,4 @@ public function getMetadata(): array | |||
{ | |||
return $this->metadata; | |||
} | |||
|
|||
public function withMetadata(array $metadata): self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message handlers may add metadata into the message
@@ -29,21 +23,4 @@ public function testFromData(): void | |||
$this->assertArrayHasKey('meta', $envelope->getMetadata()); | |||
$this->assertSame('data', $envelope->getMetadata()['meta']); | |||
} | |||
|
|||
public function testWithMessage(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this better than FooEnvelope::fromMessage($message)
or new FooEnvelope()
?
@viktorprogger benchmark is looking way better now. How about merging master into the PR branch? |
# Conflicts: # src/Message/EnvelopeTrait.php # src/Message/IdEnvelope.php # src/Middleware/FailureHandling/FailureEnvelope.php # tests/Unit/Message/IdEnvelopeTest.php
…act-class # Conflicts: # phpbench.json
No description provided.