search

vosmiic / jellyfin-ani-sync

Synchronize anime watch status between Jellyfin and anime tracking sites.
GNU General Public License v3.0
214 stars 15 forks source link

Fix Shikimori provider issues #111

Closed tie closed 4 months ago

tie commented 4 months ago

Fixes #107, fixes #110

tie commented 4 months ago

110 can be fixed more efficiently with Shikimori GraphQL API. I've marked the PR as a draft for now until I implement this.

tie commented 4 months ago

I haven’t verified rewatching and adding new titles to the watching list, but otherwise seems to be working fine. https://shikimori.one/Nia+Teppelin/history/logs

Note though that the implementation in this PR currently does not use GraphQlHelper class. It had some limitations that I wanted to avoid refactoring to move forward with the fix (e.g. no error handling).

vosmiic commented 4 months ago

Thanks for this PR, using GraphQL is pretty much always better in terms of efficiency so these changes are great. I have changed the Shikimori unit tests to reflect these changes and they are all passing now.

Any reason for removing the JSON converter functionality? I think the only reason to keep certain fields like ID as string is because it only accepts string's, and the actual ID is always an int (I believe, I could be wrong though). I feel like its better to natively treat it as an int, so there is no confusion on what the actual data type is. I'm fully open to removing it though if you think its better practice.

tie commented 4 months ago

I feel like IDs being integers is an implementation detail. There are no operations that actually rely on the fact that IDs are integers, API returns and accepts them as strings, and keeping them as strings throughout the plugin code makes the code a bit more flexible in terms of providers that can be supported (not all APIs use sequential integers for IDs, e.g. it is also common to have UUID strings).

vosmiic commented 4 months ago

Okay, fair enough. I can't see any issues with this, I haven't tested it myself as I can't retrieve my Shikimori account but since you have given it a go I think its safe to merge in. Thanks again for the contributions 👍

I am still planning on making some changes so I don't think a new update will be going out any time soon containing these fixes, so if you would like to use these features I would suggest just building your own version.