Make DeepCopy API final and immutable #121
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Slamdunk
suggested changes
Jun 12, 2018
There was a problem hiding this comment.
/cc @Slamdunk
Next time you can just ask for a review from the dedicated button ;)
This is not so much of a big change but it ensures there is only one point of injection which is preferable IMO.
That said the value is debatable
Immutability is always a value 👍
src/DeepCopy/deep_copy.php
Outdated
| function deep_copy($value, $useCloneMethod = false) | ||
| { | ||
| return (new DeepCopy($useCloneMethod))->copy($value); | ||
| public function deep_copy( |
There was a problem hiding this comment.
public here is a syntax error
Slamdunk
suggested changes
Jun 12, 2018
|
ready for review |
Slamdunk
suggested changes
Jun 22, 2018
| bool $useCloneMethod = false, | ||
| bool $skipUncloneable = false, | ||
| array $filters = [], | ||
| array $typeFilters = [] |
There was a problem hiding this comment.
Maybe we can upgrade this typehint to iterable and in deep_copy function too.
| * @param bool $skipUncloneable If enabled, will not throw an exception when coming across an uncloneable | ||
| * property. | ||
| * @param Array<Filter, Matcher> List of filter-matcher pairs | ||
| * @param Array<TypeFilter, TypeMatcher> List of type filter-matcher pairs |
There was a problem hiding this comment.
I don't like very much the use of generics in PHPDoc because there is zero support for them into the language.
Someone could interpret this as a key-value pair, which it isn't here.
|
Sure. but it’s better that just “array” where there is nothing and there is
the comment right next to it to explain
…On Fri 22 Jun 2018 at 08:34, Filippo Tessarotto ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/DeepCopy/DeepCopy.php
<#121 (comment)>:
> */
- public function __construct(bool $useCloneMethod = false)
- {
+ public function __construct(
+ bool $useCloneMethod = false,
+ bool $skipUncloneable = false,
+ array $filters = [],
+ array $typeFilters = []
Maybe we can upgrade this typehint to iterable
<https://secure.php.net/manual/en/language.types.iterable.php> and in
deep_copy function too.
------------------------------
In src/DeepCopy/DeepCopy.php
<#121 (comment)>:
> private $useCloneMethod;
/**
- * @param bool $useCloneMethod If set to true, when an object implements the __clone() function, it will be used
- * instead of the regular deep cloning.
+ * @param bool $useCloneMethod If set to true, when an object implements the __clone() function, it will
+ * be used instead of the regular deep cloning.
+ * @param bool $skipUncloneable If enabled, will not throw an exception when coming across an uncloneable
+ * property.
+ * @param Array<Filter, Matcher> List of filter-matcher pairs
+ * @param Array<TypeFilter, TypeMatcher> List of type filter-matcher pairs
I don't like very much the use of generics in PHPDoc because there is zero
support for them into the language.
Someone could interpret this as a key-value pair, which it isn't here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#121 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE76gRTVSt2xpn_d6_3SzkSeNSrGXxGnks5t_I_ngaJpZM4UjjBH>
.
|
|
I guess there might be a conflict with #131 on the immutability subject |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is not so much of a big change but it ensures there is only one point of injection which is preferable IMO.
That said the value is debatable and there is also the question of it is worth to backport those changes to 1.x with deprecation messages.
Closes #68
/cc @Slamdunk