Skip to content
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

jarek/7481: repr of scala unit object as MIMEContainer.HIDDEN #7694

Merged
merged 2 commits into from
Jul 23, 2018

Conversation

jaroslawmalekcodete
Copy link
Contributor

No description provided.

@LeeTZ LeeTZ requested a review from lmitusinski July 16, 2018 13:15
private def isUnitObject() = {
val print = interpreter.lastRequest.lineRep.call("$print").toString.trim
print.equals("")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The method name isUnitObject doesn't match what it actually does. Maybe isPrintStringEmpty? (and then use isEmpty instead of equals("")). Or just compare the value to () and ignore the interpreter's notion of how things should be printed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by isUnitObject I want to express whether the interpreter returns Unit object or not.
I used val print = interpreter.lastRequest.lineRep.call (" $ print "). toString.trim print.equals ("") because I have not found a better way to do this.
The question is whether my solution meets the requirements and doesn't provide errors.
There is a test that checks if Unit object is represented by MIMEContainer.HIDDEN https://github.com/twosigma/beakerx/pull/7694/files#diff-7a177fbeb8a50f57cde772923fc6e1ccR60
if you know use cases which we should handle or you see problems with this solution, please let us know, your input is welcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just did some exploration and the behavior of the interpreter is surprising. Calling $result when the input is "()" returns null instead of a boxed Unit, so looking at the result value alone is not a solution. I am persuaded that it makes sense to use $print to decide whether to hide the result. That said, I still think the name isUnitObject is misleading, and would suggest changing it to shouldHideResult. For a specific example, the input "(): Any" gives a result that is a unit object (with a wider type), but the interpreter does not suppress the output in that case.

@LeeTZ LeeTZ merged commit 3958b63 into master Jul 23, 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.

None yet

4 participants