-
Notifications
You must be signed in to change notification settings - Fork 70
Overload IOException constructor #276
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
public actual constructor(message: String?) : this(message, null) | ||
|
||
public actual constructor() : this(null) | ||
public actual constructor(cause: Throwable?) : this(null, cause) |
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.
The usual behavior of this exception constructor is this(cause?.toString(), cause)
.
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'd say it should be super(cause)
, and all other ctors should delegate to super ctors too.
Like here https://github.com/JetBrains/kotlin/blob/master/libraries/stdlib/wasm/src/kotlin/Exceptions.kt
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.
Secondary constructors call this
for the primary constructor here, and this
delegates to super
, so we can't call super directly.
If we have to migrate the style to delegate to call supers, it would be great to address the fix to a follow-up, and remove primary constructors.
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.
Yeap, we need to get rid of the primary ctor. I believe it could be done here along other changes, but don't mind making the change separately.
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'll send a new PR after this gets merged.
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's in #281.
@Goooler thanks for opening the PR! |
4ec4006
to
388d25e
Compare
Thank you @Goooler! |
Closes #271.