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

Remove [RegisterSerializer] and support for correspondig Register method #2941

Merged
merged 2 commits into from
Apr 14, 2017

Conversation

ReubenBond
Copy link
Member

Removes the deprecated & obsolete [RegisterSerializer] attribute and support for the corresponding Register method.

Also fixed the formatting in GrainAttributes.cs since I dislike always having to undo the auto-formatting whenever I make a change.

Fixes #2610

@jdom
Copy link
Member

jdom commented Apr 14, 2017

Looks like we implemented option 2 of #2610 (and didn't release it yet, as in wasn't in 1.4.1) but now we are going all the way with option 1. Is my understanding correct?

@ReubenBond
Copy link
Member Author

@jdom the attribute is marked as Obsolete in v1.4.1: https://github.com/dotnet/orleans/blob/v1.4.1/src/Orleans/Core/GrainAttributes.cs#L288

We:

  • Obviated the Register method via the [Serializer(...)] attribute (second part of option 1)
  • Obsoleted [RegisterSerializer]
  • Allowed Register to still work for the time being, but with a necessary modification to the signature < maybe we shouldn't have done this and made the obsoletion message an error instead of a warning.

So now we're doing the first part of option 1.

@jdom
Copy link
Member

jdom commented Apr 14, 2017

Sure, that makes sense, I was just saying that this part:

Allowed Register to still work for the time being, but with a necessary modification to the signature < maybe we shouldn't have done this and made the obsoletion message an error instead of a warning.

was actually never released. If you look at the code in v1.4.1, it was still looking for a parameterless Register method. I wonder why we didn't go all the way with option 1, since by looking at the issue, it was agreed in January. Still, looks good, merging, I just wanted to understand

@ReubenBond
Copy link
Member Author

1.4.1 still had static SerializationManager which means registration didn't need to break.

@jdom jdom merged commit b980314 into dotnet:master Apr 14, 2017
@ReubenBond ReubenBond deleted the remove-seralizer-register-method branch April 27, 2017 23:25
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove SerializationManager.Register method
3 participants