Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jan 23, 2019

As discussed in the comments of #57 - this PR changes ObjectStreamSink::promise to resolve with an array, instead of an unexpected SplObjectStorage.

@WyriHaximus WyriHaximus added this to the v0.2.0 milestone Jan 23, 2019
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Thanks for filing this PR, I agree that using an array makes much more sense here 👍

It looks like this is only a minor change in the internal APIs, but this seems to have a bigger impact on the user-facing APIs. I'm fine with this change, but can we get a list of affected functions for our upgrade guides and ensure they are all covered? Accordingly, docs and examples might have to be updated, e.g. https://github.com/reactphp/filesystem#list-contents.

@clue clue added the BC break label Jan 24, 2019
@ghost
Copy link
Author

ghost commented Jan 24, 2019

Affected by this change are the following methods:

  • AdapterInterface::ls (needs to be upgraded by users due to promises anyway)
  • Node\NodeInterface::copy (Directory, File and Link node instances)
  • Node\DirectoryInterface::lsRecursive (Directory node instances)

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@CharlotteDunois Thanks for the quick update, LGTM! 👍

@clue clue changed the title Let ObjectStreamSink::promise resolve with an array Resolve with array instead of SplObjectStorage (AdapterInterface::ls(), Node\DirectoryInterface::lsRecursive(), Node\NodeInterface::copy, ObjectStreamSink::promise()) Jan 24, 2019
@WyriHaximus WyriHaximus merged commit 4e2af61 into reactphp:master Jan 24, 2019
@ghost ghost deleted the sink-array branch January 24, 2019 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants