Initial support for export-hook #22
Conversation
|
All snapshot dependencies now replaced with releases, so this is ready to merge now. |
| } | ||
|
|
||
| @imports[ConsK] | ||
| trait ConsK0 |
There was a problem hiding this comment.
Should this be private[alleycats] sealed trait ConsK0? I don't know if that causes any issues with export hook. But in general I think that if a trait just exists as an implementation detail then we shouldn't expose it for extension/usage by users of the library. If nothing else, making it private makes it obvious to users that they aren't meant to try to use it.
There was a problem hiding this comment.
This comment applies to various other places in this PR as well.
There was a problem hiding this comment.
My default position is that all pure code should be public unless there's a compelling reason to the contrary. I don't think there is here, so I wouldn't make this private as a matter of course.
There was a problem hiding this comment.
We have a huge scala infrastructure at work, so fighting binary incompatibilities in library versions is usually my compelling reason to not expose something that is kind of an internal detail :)
There was a problem hiding this comment.
Agreed that there's a discussion to be had here which goes beyond the scope of this PR. Specifically though, I don't see any binary incompatibility issues arising from this trait.
ghost
mentioned this pull request
|
Side-stepping the larger issues around public/private, this PR seems good to me. I'd be happy to merge it and then change stuff later if/when we decide it is necessary. 👍 |
|
👍 |
Initial support for export-hook
Once this is in Kittens will build and we'll have a clearer picture of how this is playing out.