Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jan 25, 2019

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.

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 + maybe making all file descriptors a string in a future PR isn't a bad idea

@ghost
Copy link
Author

ghost commented Jan 31, 2019

@clue 🏓

@clue
Copy link
Member

clue commented Feb 2, 2019

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: 👍

@ghost
Copy link
Author

ghost commented Feb 2, 2019

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
Copy link
Member

clue commented Feb 2, 2019

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

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:

@clue clue added the BC break label Feb 2, 2019
@clue clue merged commit 36f25ca into reactphp:master Feb 2, 2019
@ghost ghost deleted the interfaces branch February 2, 2019 16:03
@WyriHaximus
Copy link
Member

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.

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