Skip to content

Add None.toString#16

Merged
artem-zinnatullin merged 3 commits intogojuno:masterfrom
bmaslakov:master
Mar 18, 2018
Merged

Add None.toString#16
artem-zinnatullin merged 3 commits intogojuno:masterfrom
bmaslakov:master

Conversation

@bmaslakov
Copy link
Copy Markdown
Contributor

I often use this class for debugging, and calling something like println("sessionId=$optional") gives this:

sessionId=com.gojuno.koptional.None@7ce026d3

Why don't we override toString? :)

sessionId=None

@bmaslakov
Copy link
Copy Markdown
Contributor Author

As a side note, it'd be also great to have Some(123) instead of Some(value=123)


data class Some<out T : Any>(val value: T) : Optional<T>()
object None : Optional<Nothing>()
object None : Optional<Nothing>() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe add an empty line between classes now?

With additional code it now looks a little bit crumpled :)

@artem-zinnatullin
Copy link
Copy Markdown
Contributor

I guess the only use case where com.gojuno.koptional.None@7ce026d3 is useful is dealing with scenarios like https://youtrack.jetbrains.com/issue/KT-14540 when you accidentally end up having multiple instances of object

But I'd still merge this PR :)

@bmaslakov
Copy link
Copy Markdown
Contributor Author

Fixed the line separator and added a cleaner version of Some.toString.
As for KT-14540, I think the proper workaround would be to add None.equals(other) = other is None, but I think it is really out of scope for this PR.

Copy link
Copy Markdown
Contributor

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's add tests? :)

data class Some<out T : Any>(val value: T) : Optional<T>()
object None : Optional<Nothing>()
data class Some<out T : Any>(val value: T) : Optional<T>() {
override fun toString() = "Some($value)"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add simple unit tests btw?

}

object None : Optional<Nothing>() {
override fun toString() = "None"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here

@bmaslakov
Copy link
Copy Markdown
Contributor Author

Did my best :)

Copy link
Copy Markdown
Contributor

@artem-zinnatullin artem-zinnatullin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOICE


}

context("Some<Object>.toString") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems unnecessary, but we can leave it :)

@artem-zinnatullin
Copy link
Copy Markdown
Contributor

@ming13 @nostra13 PTAL

Copy link
Copy Markdown
Contributor

@nostra13 nostra13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never gone to review before but this time I'm definitely going to vote for Tostringin. Candidate for release!

@artem-zinnatullin
Copy link
Copy Markdown
Contributor

MAYBE WE EVEN NEED TO BUMP MAJOR VERSION

@artem-zinnatullin artem-zinnatullin merged commit 63be277 into gojuno:master Mar 18, 2018
MaTriXy added a commit to MaTriXy/koptional that referenced this pull request Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants