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

Hotfix for 1.2 #56

Merged
merged 5 commits into from
Nov 6, 2019
Merged

Hotfix for 1.2 #56

merged 5 commits into from
Nov 6, 2019

Conversation

fanlei
Copy link
Contributor

@fanlei fanlei commented Nov 3, 2019

  1. Fix the magic method name for unset()
  2. Remove const MIN_CAPACITY assignment in Queue
  3. Assign the current stamp to the instance copy
  4. Fix documentation content.

Remain the current value of property stamp to gurantee the new instance copy with "First in, first out" ordering been preserved for values with the same priority.
It's not necessary to assign the const MIN_CAPACITY again since MIN_CAPACITY has been already handled in the Deque composited.
IDE such as Eclipse will raise documentation syntax errors if not a valid type after @return or @param.
Should be 'value' for lookupValue()
src/Hashable.php Outdated
@@ -26,7 +26,7 @@ function hash();
* Determines if two objects should be considered equal. Both objects will
* be instances of the same class but may not be the same instance.
*
* @param $obj An instance of the same class to compare to.
* @param $obj - An instance of the same class to compare to.
Copy link
Member

Choose a reason for hiding this comment

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

Why this -?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no type after @param so IDE such as Eclipse PDT would raise a syntax error in the doc block. However, I confess I was not sure what type to put there at the time when I was trying to fix it.
I checked further now and supposed $obj could be everything, according to php-ds/ext-ds#140. Do you think it's better to be 'mixed' there instead?

Copy link
Member

@rtheunissen rtheunissen left a comment

Choose a reason for hiding this comment

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

This PR is great ✨

Happy to approve if we can remove that hyphen - appears to be inconsistent.

@fanlei
Copy link
Contributor Author

fanlei commented Nov 4, 2019

Thank you for reviewing. Yes, I should keep it consistent.

@rtheunissen rtheunissen merged commit 5980373 into php-ds:master Nov 6, 2019
@fanlei fanlei deleted the hotfix-1.2 branch November 6, 2019 18:36
@fanlei fanlei restored the hotfix-1.2 branch November 6, 2019 18:38
@fanlei fanlei deleted the hotfix-1.2 branch November 6, 2019 18:39
@rtheunissen
Copy link
Member

The tests and versioning is a bit of a mess it seems - haven't looked at this in a while. I'd like to consolidate all the components as part of a 1.3.0 release.

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.

2 participants