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

Public Group API prepared for soon-to-arrive Group::remove_table() [BREAKS PUBLIC API] #515

Merged
merged 7 commits into from Aug 14, 2014

Conversation

Projects
None yet
2 participants
@kspangsege
Copy link
Contributor

kspangsege commented Aug 8, 2014

The public Group API has been heavily upgraded to fix old issues, and to properly support the soon-to-be-added Group::remove_table().

The most important change is that Group::get_table() no longer creates tables that are not already in the group. Group::get_table() will now return null if there is no table in the group with the specified name.

New tables are added with one of the new functions Group::add_table() or Group::get_or_add_table().

These changes will require a careful examination and update of all our language bindings, and it is not likely that the compiler is going to help find the places where changes are needed.

A simpleminded approach to updating a language binding would be to find every occurrence of a call to Group::get_table() and replace it with a call to Group::get_or_add_table(). This will very likely work, because Group::get_or_add_table() has virtually the same meaning as Group::get_table() had before.

Here is a detailed account of the publicly visible changes:

  • Group::has_table<T>() removed, because it had awkward and incongruous semantics, and could not be efficiently implemented.
  • Group::find_table() added as a way of mapping a table name to the index of table in the group.
  • Group::get_table(index) and Group::get_table_name() now throw InvalidArgument if the index is invalid.
  • Group::get_table(name) no longer creates a table when none exist with the specified name. Its semantics has also changed in several other ways.
  • Group::add_table() and Group::get_or_add_table() added.
  • Renaming LangBindHelper::bind_table_ref() to LangBindHelper::bind_table_ptr(), and LangBindHelper::unbind_table_ref() to LangBindHelper::unbind_table_ptr().
  • There is now two versions of LangBindHelper::get_table(), one whose argument is a table name, and one whose argument is the group-level index. Neither one is exactly like that original version of LangBindHelper::get_table().
  • LangBindHelper::add_table() and LangBindHelper::get_or_add_table() added.
  • WriteTransaction::add_table() and WriteTransaction::get_or_add_table() added.
  • Exception type ResourceAllocError eliminated, as there was no good reason for keeping it (it had no clear role).

@finnschiermer @astigsen @rrrlasse

kspangsege added some commits Aug 8, 2014

The public Group API has been heavily updated to fix old issues, and …
…to properly support the soon to be added Group::remove_table()
Fixes for: The public Group API has been heavily updated to fix old i…
…ssues, and to properly support the soon to be added Group::remove_table()
Fixes for: The public Group API has been heavily updated to fix old i…
…ssues, and to properly support the soon to be added Group::remove_table()
Merge branch 'master' into ks-upgrade-group-api
Conflicts:
	release_notes.md
@kspangsege

This comment has been minimized.

Copy link
Contributor

kspangsege commented Aug 12, 2014

Please review

@bmunkholm

This comment has been minimized.

Copy link
Contributor

bmunkholm commented Aug 12, 2014

Before merging we need to discuss the integration of this in bindings.

I really don't like to change the semantics of existing methods without getting some compiler assistance to detect it. I'm not aware if this has been discussed in advance of all these changes? Never the less, I've always argued for this more clear meaning of get_table and get_or_add_table..

@kspangsege

This comment has been minimized.

Copy link
Contributor

kspangsege commented Aug 12, 2014

I understand your concern with respect to the lack of compiler assistance. On the other hand, using grep -rnw get_table, and being just a little systematic/careful, will suffice for a reliable update. I strongly recommend that we go ahead with this, as it is a significant improvement of the group API, and even more so when considering coming additions to the group API.

If you want my help, I can general a "high fidelity" list of places where Group::get_table() is called, for Cocoa and Java, then you can consider in each case whether the right new function to call is get_table(), add_table(), or get_or_add_table().

And remember that it is always safe to substitute get_or_add_table(), because it has the same function as Group::get_table() had before, but it is not always the optimal choice, given the new possibilities.

@bmunkholm

This comment has been minimized.

Copy link
Contributor

bmunkholm commented Aug 12, 2014

Let's consider if we need to make a "binding conversion note"

kspangsege added a commit to realm/realm-cocoa that referenced this pull request Aug 13, 2014

@kspangsege

This comment has been minimized.

Copy link
Contributor

kspangsege commented Aug 13, 2014

I have prepared a PR to upgrade the Cocoa binding: realm/realm-cocoa#785

kspangsege added a commit to realm/realm-java that referenced this pull request Aug 13, 2014

@kspangsege

This comment has been minimized.

Copy link
Contributor

kspangsege commented Aug 13, 2014

I have prepared a PR to upgrade the Java binding: realm/realm-java#324

@kspangsege

This comment has been minimized.

Copy link
Contributor

kspangsege commented Aug 13, 2014

I have prepared a PR to upgrade the Python binding: Tightdb/tightdb_python#196

@kspangsege

This comment has been minimized.

Copy link
Contributor

kspangsege commented Aug 13, 2014

Please review

@kspangsege

This comment has been minimized.

Copy link
Contributor

kspangsege commented Aug 14, 2014

Please review

kspangsege added a commit that referenced this pull request Aug 14, 2014

Merge pull request #515 from Tightdb/ks-upgrade-group-api
Public Group API prepared for soon-to-arrive Group::remove_table() [BREAKS PUBLIC API]

@kspangsege kspangsege merged commit 00c35d4 into master Aug 14, 2014

1 check passed

default Merged build finished.
Details

@kspangsege kspangsege deleted the ks-upgrade-group-api branch Aug 20, 2014

kspangsege added a commit to realm/realm-cocoa that referenced this pull request Aug 20, 2014

kspangsege added a commit to realm/realm-cocoa that referenced this pull request Aug 22, 2014

jpsim added a commit to realm/realm-cocoa that referenced this pull request Aug 22, 2014

Merge branch 'master' into jp-xcode6-beta6
* master:
  Improve the migration documentation more
  Fix some typos
  Improve the migration documentation
  Fix a bunch of small errors in the RLMRealm docs
  Improve the autorefresh documentation
  Don't set TIGHTDB_MAX_LIST_SIZE for debug builds
  Upgrade core library version 0.82.0 -> 0.82.1
  Fix crash when adding an object already in a realm to another realm
  Update changelog
  Updates necessitated by change core Group API (realm/realm-core#515)
  Upgrade core library version 0.81.0 -> 0.82.0
  Use an exception rather than assertion
  Check the token's block in case it was niled while being enumerated
  Make sure the token block is not nil

Conflicts:
	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment