-
Notifications
You must be signed in to change notification settings - Fork 31
Copy type on transparent wrapping and hide type modifier transparent #13
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
Conversation
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.
I've given it a thought, and this looks good to me.
The concept of transparent wrap evolved along with errorx, and at this point it is indeed something best left mostly hidden. It is most probably a bad idea to add traits via transparent wrap. Such error type names are not present in error message and may not be checked against.
With this in mind, the idea to make this modifier private seems OK.
It is also OK to copy the type, as there is no way to observe the wrapped type anyway.
One thing that still can be done via this kind of wrap is adding property. I can see no reason to remove this, and this PR leaves it intact.
It is best to leave the tests: transparency modifier is private now, but it still uses the mechanism.
As for ErrorBuilder.Transparent()
, I have no strong arguments for its preservation or removal. It is not broken in any way, so we may just as well leave it there.
Hiding
There are four ways to enable it:
|
// TypeModifierTransparent is a type modifier; an error type with such modifier creates transparent wrappers by default | ||
TypeModifierTransparent TypeModifier = 1 | ||
// typeModifierTransparent is a type modifier; an error type with such modifier creates transparent wrappers by default | ||
typeModifierTransparent TypeModifier = 1 |
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.
It is kinda weird to have public and private type modifiers together in one public place. Maybe move typeModifierTransparent
elsewhere?
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.
I thought about removing TypeModifierTransparent
at all, and using Builder.Transparent()
method instead.
What do you think?
Or do not change its access mode at all to preserve api compatibility (see @g7r comment)
@g7r I can see no benefit from transparency in this particular example. I could conjure up another example that would be trickier in this regard, but if we struggle to find one, then I'd stick to option 4 just now. |
|
We seem to have abandoned this PR. |
Currently, transparent types are completely inaccessible, and they don't affect any thing.
Therefore there is no need to keep transparent type in an error, and no need to traverse transparent cause chain to reach the type, because it is simpler just to copy type.
Probably this PR is not complete:
Builder.Transparent()
be removed or not? Or shouldtypeModifierTransparent
be removed?Note: I believe transparent type could be used for injecting new Traits into error (after modification in
Error.HasTrait
function), and then this PR is not valid.