Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Implement no op for room stream in sync #2022
Conversation
erikjohnston
added some commits
Mar 15, 2017
All checks have passed
Some checks were not successful
erikjohnston
assigned
NegativeMjark
Mar 16, 2017
| @@ -818,6 +831,38 @@ def handle_room_entries(room_entry): | ||
| defer.returnValue((newly_joined_rooms, newly_joined_users)) | ||
| @defer.inlineCallbacks | ||
| + def _have_rooms_changed(self, sync_result_builder): | ||
| + """Returns whether any rooms have changed since the sync. Must be an | ||
| + incremental sync |
NegativeMjark
Mar 16, 2017
•
Contributor
Would the doctring be clearer if it was written something like this?
"""Returns true if there are any new events in the rooms the user is in since the last sync
or if the user has changed membership in any room since the last sync.
This only makes sense when calculating an incremental sync."""| @@ -274,24 +274,25 @@ def _get_members_rows_txn(self, txn, room_id, membership=None, user_id=None): | ||
| return rows | ||
| - @cached(max_entries=500000, iterable=True) | ||
| + @cachedInlineCallbacks(max_entries=500000, iterable=True) | ||
| def get_rooms_for_user(self, user_id): |
NegativeMjark
Mar 16, 2017
Contributor
Would it make sense to have some docstring along the lines of:
"""Returns an immutable set of room_id strings for the rooms the user is joined to."""| @@ -765,6 +764,21 @@ def _generate_sync_entry_for_rooms(self, sync_result_builder, account_data_by_ro | ||
| ) | ||
| sync_result_builder.now_token = now_token | ||
| + # We check up front if anything has changed, if it hasn't then there is | ||
| + # no point in going futher. |
erikjohnston
Mar 16, 2017
Owner
It feels like that sort of comment would get out of date quickly, and wouldn't necessarily help that much.
|
Looks good codewise. I think the comments could be improved slightly but overall it looks pretty good. |
Some checks were not successful
continuous-integration/travis-ci/pr
— The Travis CI build could not complete due to an error
|
Updated comments. |
erikjohnston
merged commit 248eb46
into
develop
Mar 16, 2017
0 of 6 checks passed
Sytest Dendron (Merged PR)
Build started sha1 is merged.
Details
Sytest Postgres (Commit)
Build #2523 origin/erikj/no_op_sync in progress...
Details
Sytest Postgres (Merged PR)
Build started sha1 is merged.
Details
Sytest SQLite (Merged PR)
Build started sha1 is merged.
Details
continuous-integration/travis-ci/pr
The Travis CI build is in progress
Details
continuous-integration/travis-ci/push
The Travis CI build is in progress
Details
erikjohnston
deleted the
erikj/no_op_sync
branch
Oct 26, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
erikjohnston commentedMar 16, 2017
No description provided.