search

reactphp / filesystem

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

Add AdapterInterface getters and add Filesystem method to interface #59

Closed ghost closed 5 years ago

ghost commented 5 years ago

This PR adds getters for the setters to the adapter interface (this allows future functional tests to validate the action), and adds the constructLink method from the Filesystem class to the interface.

This PR also changes the filedescriptor type from resource to mixed in the docblocks. De facto the file descriptor is an integer currently. But it might be any other type (such as string) from future adapters (de facto string for my pthreads adapter).

Additionally I've taken the freedom to update the readme a little bit.

ghost commented 5 years ago

@clue :ping_pong:

clue commented 5 years ago

This PR adds getters for the setters to the adapter interface (this allows future functional tests to validate the action), and adds the constructLink method from the Filesystem class to the interface.

@CharlotteDunois Thanks for filing this PR and the ping, I can see that it contains some reasonable changes, but tbqh I'm not sure I follow the motivation for adding these methods, so perhaps you can elaborate?

Full disclosure: I'm not sure the distinction between adapter interface and filesystem interface provides much value from a consumer's perspective in the first place, so (personally) I'd rather see these interfaces merged in the future. Is this longer-term vision something that is compatible with this suggested change?

Either way, I very much value your ongoing contributions and believe this keeps raising interesting questions for this projects, so keep it coming! As such, I don't mean to block this development, I'm genuinely interested in the longer-term vision here. :shipit: :+1:

ghost commented 5 years ago

Thanks for filing this PR and the ping, I can see that it contains some reasonable changes, but tbqh I'm not sure I follow the motivation for adding these methods, so perhaps you can elaborate?

They're mainly for functional unit tests (rather than having completely mocking unit tests). But I'm using this functionality myself to direct invoke a specific method without the further abstraction the equivalent adapter method provides (in my case it's ls, but since I only need the direct result of the adapter's readdir and getting more would only be unnecessary overhead, I invoke it directly through the invoker (having a queued invoker would allow throttling)).

Full disclosure: I'm not sure the distinction between adapter interface and filesystem interface provides much value from a consumer's perspective in the first place, so (personally) I'd rather see these interfaces merged in the future. Is this longer-term vision something that is compatible with this suggested change?

This would make it the same architecture as react/event-loop and I think it would make sense to merge them in the future. However we will need to rethink how much functionality we want to expose from the adapters. Whether we want to add throttling as base feature of the adapters or rather want to outsource it to a composite class would also be a question to address.

clue commented 5 years ago

@CharlotteDunois Thank you, this answers my questions and I believe we're on the same page for the longer-term vision here :v:

An architecture similar to the EventLoop is also what I had in mind and using composition to add features on top of this is likely the way forward. Let's get this PR in now and then address the remainder in follow-up PRs! Keep it coming! :shipit:

WyriHaximus commented 5 years ago

Whether we want to add throttling as base feature of the adapters or rather want to outsource it to a composite class would also be a question to address.

Been thinking about this the other day and I would rather get rid of the invokers, not sure if they add any value anymore. Initially I've added those to keep EIO under control but I feel that they shouldn't be added directly in the adapters but in another decorating or composing way instead.

Full disclosure: I'm not sure the distinction between adapter interface and filesystem interface provides much value from a consumer's perspective in the first place, so (personally) I'd rather see these interfaces merged in the future. Is this longer-term vision something that is compatible with this suggested change?

They where originally separated to have both a high and low level interface to deal with the filesystem. And I'd really prefer it to stay that way. That said I have zero issues with moving the high level side of things to a different (Friends of) ReactPHP package. The main reason for the split is so users get a more OO focused API to use.