search

microsoft / jupyter-core

Library for writing Jupyter kernels in .NET Core
MIT License
108 stars 23 forks source link

Update versioning scheme and avoid breaking interface change #62

Closed rmshaffer closed 4 years ago

rmshaffer commented 4 years ago

This PR contains two unrelated changes, but I'm combining them into one PR since both should be addressed before pushing a new Microsoft.Jupyter.Core release.

Update to assembly versioning

First, I've updated the assembly versioning scheme. The BuildId variable that had been used as part of the assembly version had become too large in the signed build pipeline and violated .NET's requirement that each version part fit into a 16-bit integer. So I've updated the assembly versioning to use a Major.Minor.0.0 scheme, which should not encounter this problem, and is also more consistent with .NET assembly versioning guidelines.

NuGet package versioning is unchanged by this. Packages will continue to use BuildId.

Backward compatibility fix

Second, I've tweaked the change to the IShellServer interface made in #59 to avoid breaking consumers who may have classes implementing it, such as this one in the iqsharp project.

rmshaffer commented 4 years ago

Thanks @cgranade for the review!

As a possible alternative, you could use C# 8's default interface methods feature to provide a new method that wraps adding and removing a delegate from an event (perhaps returning an IDisposable that unregisters the delegate, or perhaps taking an Action that is evaluated with the delegate registered), providing a reasonable fallback for shell servers without interrupt support.

That is an excellent suggestion, and I hadn't thought of that! I think in this case, if you don't mind, I'd like to leave it the way I have it in this PR because (a) this way involves a little bit less code (i.e., probably harder for me to make a mistake), (b) it's more consistent with the existing events, and (c) the new release will be pushed ASAP, so I'd like to keep the code change as small as possible (and I've already validated this with the IQ# kernel).

cgranade commented 4 years ago

All very good reasons, makes perfect sense!