Aligned behavior using build #45
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #45 +/- ##
============================================
+ Coverage 98.87% 98.88% +0.01%
- Complexity 192 196 +4
============================================
Files 9 9
Lines 531 538 +7
============================================
+ Hits 525 532 +7
Misses 6 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
luislard
changed the title
| // we build a proxy container | ||
| $container = $package->statusIs(self::STATUS_BOOTED) | ||
| || $package->statusIs(self::STATUS_INITIALIZED) | ||
| // || $package->statusIs(self::STATUS_READY) |
There was a problem hiding this comment.
Why is this line commented?
I think it makse sense to check for this status, and if we're sure it doesn't, the we should remove the line not comment it.
| $status = $failed ? 'errored' : 'booted'; | ||
| // Don't allow connect if current package is already built | ||
| if ($failed | ||
| || $this->statusIs(self::STATUS_INITIALIZED) // built |
There was a problem hiding this comment.
Same question as above: in the build() method you introduced check for "built" status checking for $this->built. Would it make sense to do this here as well?
| // we build a proxy container | ||
| $container = $package->statusIs(self::STATUS_BOOTED) | ||
| || $package->statusIs(self::STATUS_INITIALIZED) |
There was a problem hiding this comment.
And again ssame question asked twice above.
| if ($this->built) { | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
First of all, I want to ask what is the reason for this lines.
This allows for calling build() multiple times idempotently. I'm not necessarily against that, but would like to understand the reasoning.
None of the other public API of this class (boot(), connect()) is idempotent, expecting specific methods to be called at specific steps in the object life cycle.
So my questions are:
- Why this method is different?
- If there're good reasons for idempotence, do those reasons apply to other methods as well?
When we started this class, we thought about having specifc flags for different statuses (idle, ready, failed) but then we opted to have a single status property. Why is "built" a status special in this regard? Why can't we add a built status instead fo an explicit flag?
I already asked above: do we need to differentiate between STATUS_INITIALIZED and an eventual new STATUS_BUILT or we can say that STATUS_INITIALIZED previsely means that package is built?
- If we don't need to differentiate because
STATUS_INITIALIZEDmeans built, can't we check$this->statusIs(self::STATUS_INITIALIZED)instead of using this flag? - If we need to differentiate, the question is the same as above, why don't we introduce that new status and check it via
statusIs()? - If there's a good reason to have the flag instead of checking the status, why in other places are we checking the status instead of the flag for the same purpose?
| static::assertFalse($package->moduleIs($expectedId, Package::MODULE_EXTENDED)); | ||
| static::assertFalse($package->moduleIs($expectedId, Package::MODULE_ADDED)); | ||
|
|
||
| // build again should keep the status of the package. |
There was a problem hiding this comment.
I've asked in another place, but here I also don't see a reasoning.
Why it is important that we can call $package->build() multiple tiems idempotently, while for example, we can't do that for boot() or connect().
And again, I'm not necessarily againts this, I just want to understand what makes build() special in this regard.
| @@ -66,7 +66,8 @@ private function tryContainer(): bool | |||
|
|
|||
| /** TODO: We need a better way to deal with status checking besides equality */ | |||
| if ( | |||
| $this->package->statusIs(Package::STATUS_READY) | |||
| $this->package->statusIs(Package::STATUS_INITIALIZED) | |||
There was a problem hiding this comment.
Do we need to differentiate between STATUS_INITIALIZED and an eventual new STATUS_BUILT or we can say that STATUS_INITIALIZED previsely means that package is built?
In that second case, should we as an STATUS_BUILT as an alias for STATUS_INITIALIZED? And maybe even deprecate the latter?
Alos, I see you have introduced the flag built in this commit, and you use it in the build() method to check the status. If there's a good reason for doing that instead of checking the status, why don't do it as well here?
Co-authored-by: Giuseppe Mazzapica <[email protected]> Signed-off-by: Christian Leucht <[email protected]>
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
Bug: Misaligned behavior with build
The package has changed from using a boot method with passed modules to adding modules and having a build stage before boot.
Not all the tests were aligned by the time the new approach was added. I added some tests to check the current behavior and found a couple of misalignments.
Bug: Calling $package->build() several times change package status to STATUS_FAILED
What is the new behavior (if this is a feature change)?
$package->build()and the status of the package will not change.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Current consumers using the following code will have to adapt it.
Other information:
We need to find a better way to get Container Status as mentioned in here