search

ShokoAnime / Shokofin

Repository for Shokofin, a plugin that brings Shoko to Jellyfin.
https://shokoanime.com
MIT License
145 stars 17 forks source link

Update to support jellyfin 10.8.0-beta1 #20

Closed hitsounds closed 2 years ago

revam commented 2 years ago

First off, thanks for submitting a PR. The overall changes look fine but there are some changes I want you to make before I merge this. The changes involve replacing all instances of Enum.Parse<T> with their parsed value (i.e. Enum.Parse<Jellyfin.Data.Enums.BaseItemKind>(nameof (Season)) -> Jellyfin.Data.Enums.BaseItemKind.Season).

hitsounds commented 2 years ago

Thank you for the review, I have made the requested changes.

Before you merge I have the following concern. To support the jellyfin beta I had to upgrade the project to .net 6. I believe the line here https://github.com/ShokoAnime/Shokofin/blob/d2b21d5a75309148aed747ca526742ea07fe2bd6/.github/workflows/release.yml#L22 and here https://github.com/ShokoAnime/Shokofin/blob/d2b21d5a75309148aed747ca526742ea07fe2bd6/.github/workflows/release-daily.yml#L25 should be changed to 6.0.x but am not sure.

revam commented 2 years ago

I didn't even think of the CI pipeline. Good job spotting the problem early and if you can update the config files then I think we'll be ready to merge.

revam commented 2 years ago

Note for the future; add some CI checks to each PR to make sure it builds in the CI pipeline. 📝

revam commented 2 years ago

Though that's not for this PR

hitsounds commented 2 years ago

Happy to be of help! I have made the changes however have not been able to test them.

revam commented 2 years ago

Now we'll just have to see if the CI pipeline works as expected or if it fails.

revam commented 2 years ago

We forgot to change the target abi, but otherwise it looks like the pipeline worked. :+1: