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 IRealmList #880

Merged
merged 1 commit into from
Oct 25, 2016
Merged

Remove IRealmList #880

merged 1 commit into from
Oct 25, 2016

Conversation

nirinchev
Copy link
Member

@nirinchev nirinchev commented Oct 24, 2016

Seems like it's no longer necessary. Also, make RealmList internal.

Resolves #873

@nirinchev
Copy link
Member Author

@fealebenpae can you verify the dynamic API is fine like that or I'm missing something?

Copy link
Member

@fealebenpae fealebenpae left a comment

Choose a reason for hiding this comment

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

This change not only removes the IRealmList interface, but also makes RealmList<T> internal. If that's what we want to happen, you need to add a Breaking Change entry to the changelog and clean up any RealmList<T> leftovers in the weaver.

I would prefer it if we don't remove RealmList<T> without having had a release out with it being marked as deprecated, however.

@nirinchev
Copy link
Member Author

I think I've cleaned up the leftovers in the weaver, or have you noticed something that I've missed? I'll add an entry to the Changelog along with other changes that have not been added. Finally, while I generally agree with deprecating API gracefully, in this case, I believe a "nuke it" approach might work better due to the following:

  • It's a very simple class swap, so even with a large solution, it shouldn't take users more than a few minutes to fix their build.
  • We'll get deprecated warnings when building from source.

@fealebenpae
Copy link
Member

You're right - I had missed the ModuleWeaver.cs changes in this PR, I'm sorry.

I suppose we can make an exception in this case, yes. It's our fault for not having deprecated RealmList<T> when we introduced IList<T> property support.

@nirinchev nirinchev merged commit 68369c6 into master Oct 25, 2016
@nirinchev nirinchev deleted the ni/irealmlist branch October 25, 2016 16:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
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.

2 participants