Conversation
|
Rebased to address merge conflicts from b47c3b6 |
beeequeue
left a comment
There was a problem hiding this comment.
thanks for the effort, but this pr is too big to understand and merge at once. i would recommend splitting it into pieces
- changing the source json file, explaining the differences between them
- removal of the old fields
- adding new fields
- if any of these are not 1-to-1 mappings they also need new endpoints to fetch them, like tvdb has
i can probably do it if you dont have time. :)
|
|
Okay, I did split it in the 3 steps you described, but separate PRs means it will require your intervention because there will be merge conflicts and an extraneous and permanently empty |
|
Force-pushed only the addition of new fields to this PR since I can only edit the target branch, not the origin. |
| export const removeDuplicates = (entries: Relation[]): Relation[] => { | ||
| const sources = (Object.values(Source) as SourceValue[]).filter( | ||
| (source) => source !== Source.TheTVDB && source !== Source.TheMovieDB && source !== Source.IMDB, | ||
| const sources = (Object.values(Source) as SourceValue[]).filter((source) => |
There was a problem hiding this comment.
this should probably use .includes() or alternatively .has() together with changing NonUniqueFields to a Map for performance
| // eslint-disable-next-line unicorn/prefer-includes | ||
| if (NonUniqueFields.some((field) => field === source)) continue |
There was a problem hiding this comment.
same here, why disable the warning about using .includes()?
There was a problem hiding this comment.
Both cases of Skill Issue™, .includes() gave a Type Warning and I couldn't figure out how to address it properly 😅
| minimum: 0 | ||
| maximum: 50000000 | ||
| example: 1337 | ||
| animenewsnetwork: |
There was a problem hiding this comment.
please make sure that the order of fields in relations (both in docs and code) are always alphabetical
There was a problem hiding this comment.
I believe MAL was at the bottom in one place and it threw off my perception of anything else being alphabetized, I only noticed it yesterday with fresh eyes while splitting the PRs
| Source.TheMovieDBSeason, | ||
| Source.TheTVDB, | ||
| Source.TheTVDBSeason, | ||
| Source.Simkl, |
There was a problem hiding this comment.
I don't know for sure, no experience with the service but it does the whole "Global Movies and TV oh but also Anime" thing, so I preferred to err on the side of caution with non-unique
There was a problem hiding this comment.
this has to be known, since simkl will need its own endpoint like tvdb if it isnt unique
| myanimelist: id++, | ||
| simkl: id++, | ||
| animecountdown: id++, | ||
| media: `${id++}`, |
There was a problem hiding this comment.
I thought it was weird, but I followed notify.moe's model that is also alphanumerical and since the tests passed I didn't think about it
There was a problem hiding this comment.
the tests should resemble correct data, so media should be one of the allowed values instead
| animenewsnetwork: id++, | ||
| "notify-moe": `${id++}`, | ||
| themoviedb: id++, | ||
| "themoviedb-season": id++, |
There was a problem hiding this comment.
the seasons should probably be some number between 1-3 rather than an infinitely increasing number, you can hard code it for the tests
| animenewsnetwork: id++, | ||
| "notify-moe": `${id++}`, | ||
| themoviedb: specialId ?? id++, | ||
| "themoviedb-season": specialId ?? id++, |
There was a problem hiding this comment.
same here, the seasons and media shouldnt be an id
| v.string(), | ||
| v.regex(/^[\-a-z,]+$/, "Invalid `include` query"), | ||
| v.minLength(1), | ||
| v.maxLength(100), |
There was a problem hiding this comment.
With the added fields, the query length surpassed this limit.
IIRC it was a test with the include parameter and every field available
There was a problem hiding this comment.
i rewrote the schema to split the input and changed the max array length to the source count instead
| const runUpdateScript = async () => updateRelations() | ||
|
|
||
| if (NODE_ENV === "production") { | ||
| if (NODE_ENV === Environment.Prod) { |
There was a problem hiding this comment.
im not sure about this, it might be better to just remove the Environment constant altogether in another pr/commit
Don't worry about it, I haven't dabbled with TS much so such comments and pointers are more than welcome 👍 |

Fribb recently updated the lists generator, this PR aligns the output with the new output
This includes introducing a few new fields,
remapping(removed on rebase after b47c3b6), as well as retiringthetvdbthat was renamed totvdband thus failing to matchnotify-moe.I couldn't get
.alterTable()to work, hence opting to drop the entire table and recreate it on the DB migration. Didn't seem like much of a problem since it's always repopulated anywayBefore Changes (Live)
{ "anidb": 5458, "anilist": 3390, "anime-planet": "amuri-in-star-ocean", "anisearch": 4445, "imdb": null, "kitsu": 2977, "livechart": 7110, "notify-moe": null, "themoviedb": 44298, "thetvdb": null, "myanimelist": 3390 }{ "anidb": 5459, "anilist": 3269, "anime-planet": "hack-g-u-trilogy", "anisearch": 4491, "imdb": "tt1164545", "kitsu": 2895, "livechart": 4721, "notify-moe": null, "themoviedb": 8864, "thetvdb": null, "myanimelist": 3269 }After Changes
{ "anidb": 5458, "anilist": 3390, "anime-planet": "amuri-in-star-ocean", "anisearch": 4445, "imdb": null, "kitsu": 2977, "livechart": 7110, "animenewsnetwork": 8720, "themoviedb": 44298, "themoviedb-season": 1, "thetvdb": 91021, "thetvdb-season": 1, "myanimelist": 3390, "simkl": 40868, "animecountdown": 40868, "media": "OVA" }{ "anidb": 5459, "anilist": 3269, "anime-planet": "hack-g-u-trilogy", "anisearch": 4491, "imdb": "tt1164545", "kitsu": 2895, "livechart": 4721, "animenewsnetwork": 8719, "themoviedb": 8864, "themoviedb-season": null, "thetvdb": 79099, "thetvdb-season": null, "myanimelist": 3269, "simkl": 41283, "animecountdown": 41283, "media": "MOVIE" }