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

Comm Enhancements #389

Merged
merged 5 commits into from Aug 2, 2019

Conversation

@sbrunk
Copy link
Member

commented Jul 14, 2019

ipywidgets sets its protocol version in the metadata of the comm messages it sends.

This is an attempt to support setting metadata for comm messages. Sending comm messages also throws exceptions now if data or metadata can't be parsed into JSON objects (it was a TODO before in DefaultCommHandler).

@sbrunk sbrunk force-pushed the sbrunk:comm-metadata branch from d1fa273 to c51c6f9 Jul 14, 2019

@sbrunk

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2019

Is there a way to run compile or test for all major Scala Versions with a single command? I've stumbled twice upon this. I.e. because almond-spark is disabled for 2.13 and Either.toTry isn't in 2.11.

@sbrunk

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2019

mima complains because of the changes in CommHandler.

I.e.

abstract method commOpen(java.lang.String,java.lang.String,java.lang.String)Unit in class almond.interpreter.api.CommHandler does not have a correspondent in current version

But even if I keep the original commOpen and add an overloaded variant with the metadata arg, it doesn't seem to be binary compatible:

abstract method commOpen(java.lang.String,java.lang.String,java.lang.String,java.lang.String)Unit in class almond.interpreter.api.CommHandler is present only in current version

The sender method also has a default parameter value, so we can't overload it to add a metadata parameter.

@alexarchambault

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

To make mima happier, the new comm* methods should be put in a new abstract class extending the current one (like ExtendedCommHandler). And where these methods are called, we should check if we have an ExtendedCommHandler instance, and call the new methods if that's the case. For the sender method, I guess an overload, with no default values, should be added along the current one.

But overall, I'm not sure preserving binary compatibility is worth the hassle… Versions up to 0.6.0 could just be filtered out around here.

@alexarchambault

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Is there a way to run compile or test for all major Scala Versions with a single command?

+test or +test:compile ought to work.

@sbrunk

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

I'm working on a fix for #384 which brings in even more changes to CommHandler. So I'd be in favor of breaking binary compatibility.

These changes touch the sender method too so If it's OK for you I'd rename this PR to "Comm enhancements" and add these changes in the same branch.

@alexarchambault

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

@sbrunk Go ahead!

@sbrunk sbrunk changed the title Enable adding metadata to comm messages Comm Enhancements Jul 17, 2019

@sbrunk

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

Seems like I have to filter out all versions manually in

.filter(_ != "0.3.1") // Mima enabled right after it
for mima to be happy.

Would it make sense to increase the version here to 0.6.0?

val contains =
if (sv.startsWith("2.13.")) "4e9441b9"
else "v0.3.1"

@sbrunk

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

I've also added the changes related to #384 in e942f3b

alexarchambault added some commits Aug 2, 2019

@alexarchambault

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@sbrunk Should this be merged?

@sbrunk

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Yes this should be enough to fix #384

@alexarchambault

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

Let's merge then! It would have been nice to have non regression tests for that, but we can always add those later…

@alexarchambault alexarchambault merged commit b092626 into almond-sh:master Aug 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sbrunk sbrunk deleted the sbrunk:comm-metadata branch Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.