reactphp / filesystem

Evented filesystem access.
MIT License
135 stars 40 forks source link

Rename ls to lsStream and make interface-compatible ls method #57

Closed ghost closed 5 years ago

ghost commented 5 years ago

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.

ghost commented 5 years ago

@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 commented 5 years ago

@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? :+1:

WyriHaximus commented 5 years ago

@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? :+1:

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.

ghost commented 5 years ago

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

clue commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@jsor ping

jsor commented 5 years ago

@CharlotteDunois Thanks for the reminder 👍