search

reactphp / stream

Event-driven readable and writable streams for non-blocking I/O in ReactPHP.
https://reactphp.org/stream/
MIT License
626 stars 62 forks source link

Changes event handling to use constants instead of raw strings #134

Closed nawarian closed 5 years ago

nawarian commented 6 years ago

An improvement mentioned here by @michaelcullum points out using constants instead of directly using "magic" strings.

This PR updates the react/stream repo in order to do so and give a better insight about how it would look like.

Also fixed some typos and unused code.

peter279k commented 5 years ago

The changed files has conflicts, and they should be fixed.

nawarian commented 5 years ago

@peter279k conflicts resolved, build passing :)

clue commented 5 years ago

@nawarian Thanks for filing this PR, I can see where you're coming from!

Interestingly, nobody has replied to this PR, so I think it's about time. I've tried to form an opinion over this for a few months now but honestly couldn't come up with a strong argument arguments in favor of this change. I can see where it's coming from and agree that it may help with avoiding typos in event names, but other than that, what benefit does it really bring? It's my understanding this is more of an IDE-level aid that can otherwise be mitigated by unit tests, so I really haven't encountered this problem a single time when working with ReactPHP for half a decade now. This makes me wonder if it's really worth the effort and the additional (albeit little) code complexity. On top of this, this will introduce some additional opcodes for constant lookup in some performance critical code and I have yet to find the time to run some benchmarks for this. Perhaps I'm missing something and there's good reasons to want this, perhaps we can also take a look at how this is solved for Node.js and other implementations that should face similar problems?

What do you think about this?

clue commented 5 years ago

I'm having to close this for now as it hasn't received any input in a while and it's unlikely this will get traction any time soon. Please come back with more details if you think this is still relevant and we can reopen this :+1:

Thank you for your effort nonetheless, keep it up! :+1: