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

Refactor StreamWrapper and raise psalm level to 4 #221

Closed
wants to merge 3 commits into from
Closed

Conversation

vjik
Copy link
Member

@vjik vjik commented Aug 13, 2023

Q A
Is bugfix?
New feature?
Breaks BC?
Fixed issues partly #204

@vjik vjik added the status:code review The pull request needs review. label Aug 13, 2023
@codecov
Copy link

codecov bot commented Aug 13, 2023

Codecov Report

Patch coverage: 92.30% and project coverage change: +0.07% 🎉

Comparison is base (e16d64d) 73.28% compared to head (d2c50bb) 73.35%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #221      +/-   ##
============================================
+ Coverage     73.28%   73.35%   +0.07%     
+ Complexity      551      546       -5     
============================================
  Files            37       37              
  Lines          1707     1704       -3     
============================================
- Hits           1251     1250       -1     
+ Misses          456      454       -2     
Files Changed Coverage Δ
src/Collector/Stream/FilesystemStreamProxy.php 79.50% <ø> (ø)
src/Collector/Stream/HttpStreamProxy.php 43.06% <ø> (ø)
src/Helper/StreamWrapper/StreamWrapper.php 50.00% <50.00%> (+1.13%) ⬆️
src/Collector/Console/CommandCollector.php 96.92% <100.00%> (-0.05%) ⬇️
src/Collector/Console/ConsoleAppInfoCollector.php 88.88% <100.00%> (ø)
src/Collector/Web/RequestCollector.php 90.66% <100.00%> (ø)
src/Collector/Web/WebAppInfoCollector.php 90.90% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vjik vjik requested a review from a team August 13, 2023 20:34
@what-the-diff
Copy link

what-the-diff bot commented Aug 13, 2023

PR Summary

  • Error Level Modified in Psalm Configuration.
    The errorLevel attribute in psalm.xml file is changed from 5 to 4. This change means the code analysis tool will now apply a stricter set of rules, ensuring that our code quality improves.

  • Refactored Conditional Execution in CommandCollector.php.
    The condition which was only allowing certain actions to occur when $event was an instance of ConsoleEvent has been removed. Therefore, the actions within the block now execute in all cases, making the code execution simpler and more straightforward.

  • Streamlined Conditional Logic in Several Files.
    The conditional logic has been simplified in ConsoleAppInfoCollector.php, RequestCollector.php, and WebAppInfoCollector.php. The check if (!is_object($event) || !$this->isActive()) has been stripped to if (!$this->isActive()). This suggests an improvement in the system's understanding of $event and should make those areas of the code easier to read and maintain.

  • Type Hint Modifcations in FilesystemStreamProxy.php and HttpStreamProxy.php.
    The type hint for $decorated has been changed from StreamWrapperInterface to StreamWrapper, indicating an improvement in the specificity of the type of objects that can be used here.

  • Initialized property $stream in StreamWrapper.php.
    The $stream property is now initialized with false instead of null, ensuring the property always has a boolean value. It simplifies the downstream code that interacts with this property.

  • Code Optimization in StreamWrapper.php.
    The stream_rewinddir() method has been simplified and now returns a boolean directly, making our codebase more efficient.

  • Return Type Hint Added in StreamWrapperInterface.php.
    The stream_cast() method has an added return type hint. It can return either false or resource, providing clarity about what this method returns and aiding developers in handling its output.

Copy link
Member

@xepozz xepozz left a comment

Choose a reason for hiding this comment

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

Did you test the changes in src/Helper/StreamWrapper/StreamWrapper.php?
It worked in many cases I've tested and I'm not sure it's good way to change the logic without integrational tests.
PHP documentation is poor in streams context and parts of the source were taken from many other open source projects that works well. So it's better keep it as is.

src/Collector/Stream/HttpStreamProxy.php Show resolved Hide resolved
@vjik
Copy link
Member Author

vjik commented Aug 15, 2023

It worked in many cases I've tested and I'm not sure it's good way to change the logic without integrational tests.
PHP documentation is poor in streams context and parts of the source were taken from many other open source projects that works well. So it's better keep it as is.

Agree, need test. Suggest do it in separate PR. And now this package in development stage and if there are errors, then we will have the opportunity to fix them.

@xepozz
Copy link
Member

xepozz commented Aug 21, 2023

So would you like to separate the changes from src/Helper/StreamWrapper/StreamWrapper.php into a separated PR?

@vjik
Copy link
Member Author

vjik commented Aug 22, 2023

No, this related changes. But we can it sometime later.

@vjik vjik closed this Aug 22, 2023
@xepozz xepozz deleted the psalm4 branch August 22, 2023 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants