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

Sealed class inheritors in the same file #29

Closed
erokhins opened this Issue Jun 22, 2016 · 12 comments

Comments

Projects
None yet
8 participants
@erokhins
Collaborator

erokhins commented Jun 22, 2016

@norswap

This comment has been minimized.

Show comment
Hide comment
@norswap

norswap Jun 23, 2016

This would be a great thing to have.

A question though, with this definition:

package foo
sealed class A
class B: A()

I would import B with import foo.B (not the current import foo.A.B), right?
I think that's the desirable behaviour. In current Kotlin I find the need to do import foo.* then import foo.A.* rather pesky (although I understand why that might be useful).

This is what I like about this proposal: it would let the user choose whether sealed class inheritors are visible at the top-level or not.

norswap commented Jun 23, 2016

This would be a great thing to have.

A question though, with this definition:

package foo
sealed class A
class B: A()

I would import B with import foo.B (not the current import foo.A.B), right?
I think that's the desirable behaviour. In current Kotlin I find the need to do import foo.* then import foo.A.* rather pesky (although I understand why that might be useful).

This is what I like about this proposal: it would let the user choose whether sealed class inheritors are visible at the top-level or not.

@erokhins

This comment has been minimized.

Show comment
Hide comment
@erokhins

erokhins Jun 23, 2016

Collaborator

You could use import foo.B or import foo.* in this case. Why do you think, that it is desirable behavior?

Collaborator

erokhins commented Jun 23, 2016

You could use import foo.B or import foo.* in this case. Why do you think, that it is desirable behavior?

@norswap

This comment has been minimized.

Show comment
Hide comment
@norswap

norswap Jun 23, 2016

Terseness. If you have a Maybe sealed class, you usually don't want to write Maybe.Some and Maybe.None but rather Some and None directly. You can do it but it requires an additional import. And probably part of my frustration on this topic is that IntelliJ handles this in a retarded way: if you write Some, it will offer the import option (even if Maybe is already in scope) and write Maybe.Some. If you want Some on its own, you need to edit the imports manually.

Currently, I just use a private constructor for Maybe instead of a sealed class. But then you lose the when support, etc. A shame, really.

norswap commented Jun 23, 2016

Terseness. If you have a Maybe sealed class, you usually don't want to write Maybe.Some and Maybe.None but rather Some and None directly. You can do it but it requires an additional import. And probably part of my frustration on this topic is that IntelliJ handles this in a retarded way: if you write Some, it will offer the import option (even if Maybe is already in scope) and write Maybe.Some. If you want Some on its own, you need to edit the imports manually.

Currently, I just use a private constructor for Maybe instead of a sealed class. But then you lose the when support, etc. A shame, really.

@MarioAriasC

This comment has been minimized.

Show comment
Hide comment
@MarioAriasC

MarioAriasC Jun 23, 2016

I have the same problem in funKTionale

import org.funktionale.either.Either.Left
import org.funktionale.either.Either.Right
import org.funktionale.option.Option.None

https://github.com/MarioAriasC/funKTionale/blob/master/src/test/kotlin/org/funktionale/either/EitherTest.kt

I have the same problem in funKTionale

import org.funktionale.either.Either.Left
import org.funktionale.either.Either.Right
import org.funktionale.option.Option.None

https://github.com/MarioAriasC/funKTionale/blob/master/src/test/kotlin/org/funktionale/either/EitherTest.kt

@erokhins

This comment has been minimized.

Show comment
Hide comment
@erokhins

erokhins Jun 23, 2016

Collaborator

Now I see your point. I'm glad that this proposal will fix it.

Collaborator

erokhins commented Jun 23, 2016

Now I see your point. I'm glad that this proposal will fix it.

@abreslav

This comment has been minimized.

Show comment
Hide comment
@abreslav

abreslav Jul 12, 2016

Contributor

The proposal says:

Proposal: allow top-level subclasses for a top-level sealed class in the same file.

Why not just anywhere in the same file (but not a local class, of course)? I see it in the "Questions" section, but what was the reasoning behind the restriction in the first place?

Contributor

abreslav commented Jul 12, 2016

The proposal says:

Proposal: allow top-level subclasses for a top-level sealed class in the same file.

Why not just anywhere in the same file (but not a local class, of course)? I see it in the "Questions" section, but what was the reasoning behind the restriction in the first place?

@abreslav

This comment has been minimized.

Show comment
Hide comment
@abreslav

abreslav Jul 12, 2016

Contributor

So we should collect all classes inside it and, if class A is top-level, collect all classes in the same package.

I can't say I can follow this. Why the same package?

Contributor

abreslav commented Jul 12, 2016

So we should collect all classes inside it and, if class A is top-level, collect all classes in the same package.

I can't say I can follow this. Why the same package?

@abreslav abreslav closed this in d5b2a02 Jul 15, 2016

@nhaarman

This comment has been minimized.

Show comment
Hide comment
@nhaarman

nhaarman Jul 17, 2016

Perhaps this issue shouldn't have been closed.

That said, I'd like to see the following to compile:

sealed class A
sealed class B : A()

class C : B()
class D : B()

fun test(a: A): Any {
    return when (a) {
        is C -> ""
        is D -> ""
    }
}

The when expression fails to compile because the compiler thinks it is not exhaustive. Issue KT-10648 covers this, but the only comment from @mglukhikh is that this is questionable.

Perhaps this issue shouldn't have been closed.

That said, I'd like to see the following to compile:

sealed class A
sealed class B : A()

class C : B()
class D : B()

fun test(a: A): Any {
    return when (a) {
        is C -> ""
        is D -> ""
    }
}

The when expression fails to compile because the compiler thinks it is not exhaustive. Issue KT-10648 covers this, but the only comment from @mglukhikh is that this is questionable.

@erokhins erokhins reopened this Jul 19, 2016

@erokhins

This comment has been minimized.

Show comment
Hide comment
@erokhins

erokhins Jul 19, 2016

Collaborator

Why not just anywhere in the same file (but not a local class, of course)? I see it in the "Questions" section, but what was the reasoning behind the restriction in the first place?

@abreslav Well, there is some not-trivial implementations details(about generation synthetic constructors) which was solved for top-level classes but how do it in general is not clear.

I can't say I can follow this. Why the same package?

Because top-level classes from one file belong to same package.

Collaborator

erokhins commented Jul 19, 2016

Why not just anywhere in the same file (but not a local class, of course)? I see it in the "Questions" section, but what was the reasoning behind the restriction in the first place?

@abreslav Well, there is some not-trivial implementations details(about generation synthetic constructors) which was solved for top-level classes but how do it in general is not clear.

I can't say I can follow this. Why the same package?

Because top-level classes from one file belong to same package.

@sthakor1

This comment has been minimized.

Show comment
Hide comment
@sthakor1

sthakor1 May 10, 2017

Can someone post an example of inheritance with data class please?

Can someone post an example of inheritance with data class please?

@yole

This comment has been minimized.

Show comment
Hide comment
@yole

yole May 10, 2017

Contributor

@sthakor1 have you checked the official Kotlin documentation? http://kotlinlang.org/docs/reference/sealed-classes.html

Contributor

yole commented May 10, 2017

@sthakor1 have you checked the official Kotlin documentation? http://kotlinlang.org/docs/reference/sealed-classes.html

@dzharkov

This comment has been minimized.

Show comment
Hide comment
@dzharkov

dzharkov Feb 14, 2018

Collaborator

The feature has been released in Kotlin 1.1, thus I'm closing the KEEP. Do not hesitate to open a YouTrack issue for any additional suggestions.

Collaborator

dzharkov commented Feb 14, 2018

The feature has been released in Kotlin 1.1, thus I'm closing the KEEP. Do not hesitate to open a YouTrack issue for any additional suggestions.

@dzharkov dzharkov closed this Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment