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

Fix Shikimori provider issues #111

Merged
merged 7 commits into from
Mar 7, 2024
Merged

Fix Shikimori provider issues #111

merged 7 commits into from
Mar 7, 2024

Conversation

tie
Copy link
Contributor

@tie tie commented Mar 3, 2024

Fixes #107, fixes #110

@tie tie marked this pull request as draft March 3, 2024 22:06
@tie
Copy link
Contributor Author

tie commented Mar 4, 2024

#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 tie marked this pull request as ready for review March 4, 2024 15:42
@tie
Copy link
Contributor Author

tie commented Mar 4, 2024

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).

@tie tie marked this pull request as draft March 4, 2024 16:59
@tie tie marked this pull request as ready for review March 4, 2024 18:09
@vosmiic vosmiic self-requested a review March 4, 2024 23:43
@vosmiic
Copy link
Owner

vosmiic commented Mar 5, 2024

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
Copy link
Contributor Author

tie commented Mar 5, 2024

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
Copy link
Owner

vosmiic commented Mar 7, 2024

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.

@vosmiic vosmiic merged commit 0ecd408 into vosmiic:master Mar 7, 2024
3 checks passed
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.

NPE during status update with Shikimori provider Shikimori provider resets score
2 participants