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

Cleanup SyncLibraryTask #129

Merged
merged 3 commits into from
Oct 10, 2016
Merged

Cleanup SyncLibraryTask #129

merged 3 commits into from
Oct 10, 2016

Conversation

j2ghz
Copy link
Contributor

@j2ghz j2ghz commented Sep 12, 2016

I've cleaned up SyncLibraryTask and split it into multiple methods. Should I try to follow StyleCop, or try to leave as much code as possible as it is?

Includes: #108 #127

@LukePulverenti
Copy link
Member

Hi, thanks ! So this is just a refactor, no functional change?

@j2ghz
Copy link
Contributor Author

j2ghz commented Sep 18, 2016

It includes changes from #127, so I'm waiting for that before doing any more cleaning, since it touches the same part. But other than that no, it's just refactor. It's in preparation for #95. If #127 is not accepted I'll try to rebase them to master.

@LukePulverenti
Copy link
Member

Ok thanks. #127 merged in.

@j2ghz
Copy link
Contributor Author

j2ghz commented Sep 20, 2016

Should I try to follow StyleCop, or try to leave as much code as possible as it is? I'm asking because of Ordering Rules, the diff then looks like this: https://gist.github.com/j2ghz/82ec9662a121aeb66a4a0538556a433a/revisions?diff=split

@LukePulverenti
Copy link
Member

If you're going to continue to make improvements to the plugin then you can do what you're most comfortable with. We don't use style cop but if you want to make non-functional code improvements that is fine.

I think our number one wish list for this plugin would be to just have it work automatically without the need to manually run scheduled tasks.

@LukePulverenti
Copy link
Member

What do you think?

@j2ghz
Copy link
Contributor Author

j2ghz commented Oct 1, 2016

About not needing manual tasks? is it even possible to make it better? Trakt won't let us know some data changed and anytime something changes in emby it is posted to trakt I think. But I will look into it.

@LukePulverenti
Copy link
Member

Or just continuing with the current work you were planning on. I was really just trying to see if this was still moving. Thanks.

@LukePulverenti
Copy link
Member

Are these changes stable?

@LukePulverenti LukePulverenti merged commit 932c030 into MediaBrowser:master Oct 10, 2016
@LukePulverenti
Copy link
Member

Fyi, I've moved the trakt plugin to it's own dedicated repo: https://github.com/MediaBrowser/trakt

@j2ghz
Copy link
Contributor Author

j2ghz commented Oct 11, 2016

The changes are untested, but I didn't make any functional changes. It was mostly just extracting methods.

@j2ghz j2ghz deleted the feature/cleanup branch October 11, 2016 06:21
@LukePulverenti
Copy link
Member

I notice the SyncLibraryTask also updates emby watched states. Does this mean the SyncFromTrakt scheduled task is now obsolete? In other words, we now have two scheduled tasks updating watch data on the Emby side.

@j2ghz
Copy link
Contributor Author

j2ghz commented Nov 14, 2016

With the #127 option disabled (default, same as before):

SyncLibraryTask modifies Trakt
SyncFromTrakt modifies Emby.

With the option enabled:

SyncLibraryTask resets any watches, that were not scrobbled, but just marked as watched. Scrobbles are processed elsewhere (event handlers), so in this case it doesn't change Trakt data at all.
SyncFromTrakt syncs any changes from Trakt.
(basically, SyncLibraryTask sets emby items as unwatched, SyncFromTrakt sets them as watched)

The tasks could (maybe even should) be merged, but SyncFromTrakt is not obsolete.

@j2ghz j2ghz changed the title [WIP] Cleanup SyncLibraryTask Cleanup SyncLibraryTask Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants