Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jan 17, 2019

This PR makes the ls method on the adapters compatible to the interface. However instead of removing a streaming ls, a new method was added to provide this feature. That means the equivalent named methods on Node\Directory now simply delegate to the adapter methods.

Unit tests have been adapted proportionally.

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 Thank you, makes a lot more sense to me like this! 👍 (#46)

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

ghost commented Jan 18, 2019

@clue Would it make sense to make ObjectStreamSink return an array instead of a SplObjectStorage? I've remembered after going through the code again (after all I've put the tests together with glue after a rather rough day), that it resolves with an object and not an array, which would be quite unexpected by the library user. Or would it make more sense to only adapt the specific ls methods? I will add a new commit for this to distinguish that.

cc @WyriHaximus

@clue
Copy link
Member

clue commented Jan 18, 2019

@clue Would it make sense to make ObjectStreamSink return an array instead of a SplObjectStorage?

I'm not an expert for this particular repository, but I agree that an array is what I would have expected from this method. It's my understanding that this might make sense, but I would consider this to be out of scope of this PR also also perhaps @WyriHaximus can share some info here as well? 👍

@WyriHaximus
Copy link
Member

WyriHaximus commented Jan 20, 2019

@clue Would it make sense to make ObjectStreamSink return an array instead of a SplObjectStorage?

I'm not an expert for this particular repository, but I agree that an array is what I would have expected from this method. It's my understanding that this might make sense, but I would consider this to be out of scope of this PR also also perhaps @WyriHaximus can share some info here as well? 👍

Doing array for ObjectStreamSink would make perfect sense. Later on I want to change lsStream to return an RXPHP Observable anyway to make it more seamless and powerful for power users anyway tbh.

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@ghost
Copy link
Author

ghost commented Jan 20, 2019

I'll make a PR for that change after this one.

@clue
Copy link
Member

clue commented Jan 20, 2019

Later on I want to change lsStream to return an RXPHP Observable anyway to make it more seamless and powerful for power users anyway tbh.

While I can see some use for this, I'd like to discuss this more in depth before deciding on whether this makes sense. I agree a separate ticket or PR is the way to go 👍

@WyriHaximus
Copy link
Member

Later on I want to change lsStream to return an RXPHP Observable anyway to make it more seamless and powerful for power users anyway tbh.

While I can see some use for this, I'd like to discuss this more in depth before deciding on whether this makes sense. I agree a separate ticket or PR is the way to go 👍

Of course :).

@ghost
Copy link
Author

ghost commented Jan 22, 2019

@jsor ping

@jsor jsor merged commit 5ca5146 into reactphp:master Jan 23, 2019
@jsor
Copy link
Member

jsor commented Jan 23, 2019

@CharlotteDunois Thanks for the reminder 👍

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