-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Replace deprecated package #351
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #351 +/- ##
============================================
+ Coverage 94.17% 95.11% +0.93%
- Complexity 700 701 +1
============================================
Files 70 70
Lines 1837 1841 +4
============================================
+ Hits 1730 1751 +21
+ Misses 107 90 -17 ☔ View full report in Codecov by Sentry. |
0d78366
to
7ed6076
Compare
@rennokki any chance to check this pr? |
@rennokki bump |
@rennokki Sorry but I want to bump it again, please take a look when you have time |
cc @rennokki pls :) |
@@ -12,12 +12,13 @@ | |||
} | |||
], | |||
"require": { | |||
"ext-json": "*", |
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 would advise to remove this extension as it is already existing in other packages as well.
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.
We should not rely on other packages' dependencies if we are using json_*
functions in this package's code.
Check this for more details
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.
@rennokki let me know what you think, thanks.
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 got encouraged by PhpStorm to add this package as well in #380 but didn't. I'd also recommend putting it explicitly in the requires section.
I don't know how I missed this PR. I have another variant of it here: I can trim off my commit and just keep the other bumps if we favor this one... I can also merge this commit over into mine to consolidate as well. Great work! |
@rennokki Sorry to bother you again, but I'm really hope this PR can be merged, please let me know what you concern, thanks! |
bf592cb
to
4929c19
Compare
It's almost one year later... Bump again @rennokki |
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.
we'll inline the relevant code on our side
tldr; don't use this package
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.
cc @rennokki
vierbergenlars/php-semver has been deprecated and can be replaced by composer/semver.
Also add the missing json ext requirement.