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

[toggle-core#16] Create FeatureWasCreated event #24

Merged
merged 2 commits into from Oct 3, 2021
Merged

Conversation

pcs289
Copy link
Member

@pcs289 pcs289 commented Oct 3, 2021

Closes issue togge-core#16

@pcs289 pcs289 requested review from kpicaza and xserrat October 3, 2021 00:00
@codecov
Copy link

codecov bot commented Oct 3, 2021

Codecov Report

Merging #24 (97ae399) into 1.0.x (e2efb7c) will increase coverage by 1.99%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##              1.0.x      #24      +/-   ##
============================================
+ Coverage     93.22%   95.21%   +1.99%     
- Complexity       74       78       +4     
============================================
  Files            18       19       +1     
  Lines           177      188      +11     
============================================
+ Hits            165      179      +14     
+ Misses           12        9       -3     
Impacted Files Coverage Δ
src/Write/Event/FeatureWasCreated.php 100.00% <100.00%> (ø)
src/Write/Feature.php 100.00% <100.00%> (+10.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2efb7c...97ae399. Read the comment docs.

Copy link
Member

@kpicaza kpicaza left a comment

Choose a reason for hiding this comment

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

Nice work, we have to extend a little the tests and it will be ready to rebase ;-D

src/Write/Event/FeatureWasCreated.php Show resolved Hide resolved
Copy link
Contributor

@xserrat xserrat left a comment

Choose a reason for hiding this comment

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

As commented @kpicaza , we should check the type of the event recorded in order to ensure that it's the right event. Also we can discuss about passing the primitive or the VO object when creating the instance. What do you think?

Comment on lines 10 to 12
private string $eventType;

private string $featureId;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can define properties in the same order as they're assigned in the constructor:

Suggested change
private string $eventType;
private string $featureId;
private string $featureId;
private string $eventType;

public function __construct(string $featureId)
{
$this->featureId = $featureId;
$this->eventType = "FEATURE_WAS_CREATED";
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could put the string in lowercase and extract it into a constant for future references. @kpicaza what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

we don't need event type the event class name is the type itself


private string $featureId;

public function __construct(string $featureId)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could pass the FeatureId VO instead of the primitive to ensure that it's a right FeatureId. Then, inside the constructor we can assign the string value to the property.

Copy link
Member

Choose a reason for hiding this comment

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

Is better to work with generics at this level to easy serialization of the object, we create the value object in the getter as it is immutable

@@ -48,7 +48,7 @@ public function testItShouldBeEnabled(): void
$feature->enable();
$this->assertTrue($feature->isEnabled());
$events = $feature->release();
$this->assertCount(0, $events);
$this->assertCount(1, $events); // Released FeatureWasCreated event
Copy link
Contributor

Choose a reason for hiding this comment

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

As @kpicaza commented, we could add an extra line here, checking the event type, for example:

Suggested change
$this->assertCount(1, $events); // Released FeatureWasCreated event
$this->assertCount(1, $events);
$this->assertInstanceOf(FeatureWasCreated::class, reset($events));

Copy link
Member Author

Choose a reason for hiding this comment

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

We added a specific test for FeatureWasCreated event

@pcs289 pcs289 requested a review from kpicaza October 3, 2021 11:24
Copy link
Member

@kpicaza kpicaza left a comment

Choose a reason for hiding this comment

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

👏👏👏

@pcs289 pcs289 merged commit 5f92f6e into 1.0.x Oct 3, 2021
@pcs289 pcs289 deleted the feature_was_created branch October 3, 2021 11:56
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.

None yet

3 participants