-
Notifications
You must be signed in to change notification settings - Fork 789
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
Fix #557 - public type implements an internal interface that uses an internal type #1920
Conversation
@kurtschelfthout Wow, it would be great to have this fixed. The approach looks like the correct one. |
(re: edit of title: "There is no try. Do or don't." :) ) So I added more tests and moved them to a more appropriate place (I think). It seems to work without any further compiler changes. Is the CI still borked? Some things failed last push but I couldn't really find the actual problem or even whether it's something I caused. In any case this is ready for review from my perspective - I had some trouble coming up with good negative test cases or edge cases where things may go wrong, so feel free to suggest things along those lines (or anything else of course). |
@@ -0,0 +1,84 @@ | |||
|
|||
module Definitions = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be module internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be an additional test, but for the purpose of this fix I was trying to make things as clear as possible by keeping everything public except the interfaces/types themselves.
@kurtschelfthout Kevin |
I'd like to request a review for this.
The change passes the test I added, but not sure the approach is ok. If you think this is promising, I'd want to add more tests too.
Issue #557