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

Allow ^2.0 and ^3.0 of psr/log #127

Merged
merged 2 commits into from
Apr 11, 2022
Merged

Allow ^2.0 and ^3.0 of psr/log #127

merged 2 commits into from
Apr 11, 2022

Conversation

hendrikheil
Copy link
Contributor

Description

This PR seeks to add support for API-compatible versions of psr/log

Issue reference

#126

Checklist

  • Tests pass
  • Created/updated tests
  • Extended the documentation

Copy link
Contributor

@withinboredom withinboredom left a comment

Choose a reason for hiding this comment

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

LGTM, I just had a non-blocking issue if you want to fix it.

use LogicException;

/**
* Class CloudEvent
* @package Dapr\PubSub
*/
#[Deprecated(since: '1.3.0', replacement: \CloudEvents\V1\CloudEvent::class)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! It may also be worth adding a @deprecated to the docblock so other tools pick it up (like psalm/phpstan) that maybe don't know about phpstorm's attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! I'll add that right away.
However I didn't really mean to push the CloudEvent replacement into the same PR as the psr/log updates 😅

@hendrikheil
Copy link
Contributor Author

@withinboredom The psr/log update should now be good to go. I'll send another PR for the CloudEvents replacement

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2022

Codecov Report

Merging #127 (3d12c8e) into main (f0d66c3) will decrease coverage by 2.50%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main     #127      +/-   ##
============================================
- Coverage     75.99%   73.48%   -2.51%     
  Complexity      562      562              
============================================
  Files            69       72       +3     
  Lines          1791     1746      -45     
============================================
- Hits           1361     1283      -78     
- Misses          430      463      +33     
Impacted Files Coverage Δ
src/lib/Client/HttpStateTrait.php 78.00% <0.00%> (-3.04%) ⬇️
src/lib/SecretManager.php 72.72% <0.00%> (-2.28%) ⬇️
src/lib/Actors/Reminder.php 84.00% <0.00%> (-1.19%) ⬇️
src/lib/Actors/ActorConfig.php 65.51% <0.00%> (-1.15%) ⬇️
src/lib/Actors/Generators/DynamicGenerator.php 88.63% <0.00%> (-0.95%) ⬇️
src/lib/Client/PromiseHandlingTrait.php 92.30% <0.00%> (-0.55%) ⬇️
src/lib/App.php 84.73% <0.00%> (-0.46%) ⬇️
src/lib/State/TransactionalState.php 91.66% <0.00%> (-0.40%) ⬇️
src/lib/PubSub/CloudEvent.php 80.35% <0.00%> (-0.35%) ⬇️
src/lib/Serialization/Serializer.php 94.73% <0.00%> (-0.27%) ⬇️
... and 22 more

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 f0d66c3...3d12c8e. Read the comment docs.

@withinboredom
Copy link
Contributor

withinboredom commented Mar 3, 2022

I expect the integration tests to fail to build again, but while I fix them, and you're in the composer.json file, feel free to add yourself to the authors key :)

@hendrikheil
Copy link
Contributor Author

Finally some public contributions besides fixing typos and updating docs 🔥 :-)

Signed-off-by: Hendrik Heil <[email protected]>
@hendrikheil
Copy link
Contributor Author

@withinboredom Same here, just added my signature so should be ready for merging.

@withinboredom withinboredom merged commit e469a44 into dapr:main Apr 11, 2022
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.

3 participants