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

Move 'done' and 'req-done' events from sync to rs #1044

Merged
merged 5 commits into from Oct 25, 2017

Conversation

3 participants
@galfert
Copy link
Member

galfert commented Oct 25, 2017

They are called 'sync-done' and 'sync-req-done' now.

refs #732

Move 'done' and 'req-done' events from sync to rs
They are called 'sync-done' and 'sync-req-done' now.
@gregkare

This comment has been minimized.

Copy link
Member

gregkare commented Oct 25, 2017

It's missing the documentation changes in the List of events (doc/js-api/remotestorage.rst)

@galfert

This comment has been minimized.

Copy link
Member

galfert commented Oct 25, 2017

I added the documentation.

I noticed that the documentation also says that remoteStorage would emit the wire-busy and wire-done events as well. But they are emitted by remoteStorage.remote instead.

Should we move these events to remoteStorage as well?

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 25, 2017

@galfert Are they needed as public API? We're not using them in the widget, right?

@@ -76,6 +76,14 @@ List of events
"""""""""""""
Emitted when a wire request completes

``sync-req-done``
"""""""""""""""""
Emitted when a single sync task has finished

This comment has been minimized.

@skddc

skddc Oct 25, 2017

Member

What does "sync task" mean exactly?

This comment has been minimized.

@galfert

galfert Oct 25, 2017

Member

If I understand correctly, a task can be a fetch of a directory or node, or the upload of a local change.

This comment has been minimized.

@skddc

skddc Oct 25, 2017

Member

So why not say "sync request" then? The event's name already uses it and seems to be correct and descriptive in this case.


``sync-done``
"""""""""""""
Emitted when the complete sync cycle has finished

This comment has been minimized.

@skddc

skddc Oct 25, 2017

Member

As this document describes the events emitted by rs in general, what is "the complete sync cycle"? I think this is supposed to refer to a/one sync cycle/process?

@galfert

This comment has been minimized.

Copy link
Member

galfert commented Oct 25, 2017

Are they needed as public API? We're not using them in the widget, right?

They might be useful to use when caching is disabled for loading indication during up- and downloads.

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 25, 2017

Ah, right. So let's move them?

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 25, 2017

(FYI: I linked the old issue about knowing when sync is done in the PR description)

@galfert

This comment has been minimized.

Copy link
Member

galfert commented Oct 25, 2017

Moved the wire events as well and wording of the documentation a bit.


``sync-req-done``
"""""""""""""""""
Emitted when a single sync task has finished
Emitted when a sync request has finished

This comment has been minimized.

@skddc

skddc Oct 25, 2017

Member

I actually liked the "single" in there. Makes it absolutely clear what is meant. I realize I should've probably just changed it instead of commenting word by word, but I had to clear up what it refers to first.


``sync-done``
"""""""""""""
Emitted when the complete sync cycle has finished
Emitted when a sync cycle has finished

This comment has been minimized.

@skddc

skddc Oct 25, 2017

Member

Not sure if removing "complete" is improving clarity here, or making it more ambiguous. I'd probably change "finished" to "completed" even, so it's clear when this is emitted. In a comment I found earlier, it also says "when all tasks of a sync have been completed and a new sync is scheduled", making it even clearer.

This comment has been minimized.

@galfert

galfert Oct 25, 2017

Member

I like that description, will change it

@skddc

This comment has been minimized.

Copy link
Member

skddc commented Oct 25, 2017

LGTM

@skddc skddc merged commit 81f21e7 into master Oct 25, 2017

@skddc skddc deleted the feature/move_sync_events branch Oct 25, 2017

@skddc skddc removed the in progress label Oct 25, 2017

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