Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract Socket constants in separate file. #6717

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Sep 13, 2018

Socket::Address and Socket::Addrinfo conceptually don't dependent on
Socket. The shared constants and lib includes are extracted to common.cr to separate concerns more clearly.

@straight-shoota straight-shoota force-pushed the jm/refactor/socket-extract-common branch 2 times, most recently from 8de9343 to 8755a3a Compare September 13, 2018 20:04
`Socket::Address` and `Socket::Addrinfo` conceptually don't dependent on
`Socket`. The shared constants and lib includes are extracted to common.cr to separate
these concerns more cleary.
@ysbaddaden
Copy link
Contributor

I dont understand. If they're common constants, that they'll be included anyway, and they're still named Socket::X then I expect them to be defined in socket.cr not buried in some other file.

Can we avoid stylistic/opinion changes, and concentrate on fixing actual issues in Socket?

@straight-shoota
Copy link
Member Author

This is not about style, it is about separation of concerns. I'd like socket.cr to focus on the Socket type, not some other stuff that is in its namespace but not solely required by Socket.
The idea is, if you only need Socket::Address, you can just include socket/address instead of the entire package.
While this selective requiring doesn't have any relevant performance impact as it might have in other languages, the expression of explicit dependencies is still valuable.

This PR is in preparation for refactoring the entire socket API. In the process I found it was much easier to separate individual pieces of code into separate files instead of having the main type + some common stuff in the main file.

This PR is not an integral part of the refactoring, that's why I published it separately. But it makes refactoring easier.

@jhass
Copy link
Member

jhass commented Apr 29, 2020

So, where do we want to go with this? 18 months later it didn't seem to have been a pain point for anybody given the activity here...

@straight-shoota
Copy link
Member Author

Closing stale PR.

@straight-shoota straight-shoota deleted the jm/refactor/socket-extract-common branch November 19, 2020 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants