Added alternative titles #426
Conversation
dcb1be8 to
fa74456
Compare
fa74456 to
4ba83b0
Compare
There was a problem hiding this comment.
The approach looks good to me.
It does repeat values for title english and japanese. I guess we can gracefully deprecate that in the next major release? I'm thinking along the lines of grouping all titles into one object for the next major release which should handle the scaling here.
src/Model/Manga/Manga.php
Outdated
| @@ -189,6 +194,7 @@ public static function fromParser(MangaParser $parser): Manga | |||
| $instance->titleEnglish = $parser->getMangaTitleEnglish(); | |||
| $instance->titleSynonyms = $parser->getMangaTitleSynonyms(); | |||
| $instance->titleJapanese = $parser->getMangaTitleJapanese(); | |||
| $instance->alternativeTitles = $parser->getAlternativeTitles(); | |||
There was a problem hiding this comment.
@nerg4l Should this be titleAlternatives in order to match the naming order of the rest (titleEnglish, titleSynonyms, titleJapanese)?
Which would get auto serialized to title_alternatives by the REST API.
|
I think we could already group the titles and expose a |
|
@nerg4l This would be a major version change though? |
|
We would keep the current properties just instead of |
|
@nerg4l That sounds good. And then we can remove the other title properties later on. Perhaps we can call the "official title" just I could see it working like this |
|
|
5f80cbc to
e270663
Compare
|
@nerg4l You're right. Confused the response here with another's. |
|
I was planning to rename the exposed property to |
|
@nerg4l I've reverted it, can you PR it again? |
|
Tomorrow I will have time. |
This PR adds logic for parsing alternative titles. Not sure if this is the best logic for doing this so I'm open for suggestions.
Resolves #397