D11365: also test for value types in taglibextractortest and fix errors

[Matthieu Gallien]

mgallien requested changes to this revision.
mgallien added a comment.
This revision now requires changes to proceed.


  Please fix the issues.

INLINE COMMENTS

> taglibextractor.cpp:363
> +        if (!genresList.isEmpty()) {
> +            result->add(Property::Genre, genresList);
>          }

Is it really needed to push directly a list ?
You are ignoring the fact that the original developer intended add to be called once for each value. I would prefer to keep doing that until we can be absolutely sure that nothing breaks if we change that.

> taglibextractor.cpp:370
> +        const QStringList artists = contactsFromString(artistString);
> +        result->add(Property::Artist, artists);
>  

idem

> taglibextractor.cpp:374
> +        const QStringList composers = contactsFromString(composersString);
> +        result->add(Property::Composer, composers);
>  

idem

> taglibextractor.cpp:385
> +            const QStringList albumArtists = contactsFromString(albumArtistsString);
> +            result->add(Property::AlbumArtist, albumArtists);
>          }

idem

> taglibextractor.cpp:389
>          if (tags->track()) {
> -            result->add(Property::TrackNumber, tags->track());
> +            result->add(Property::TrackNumber, static_cast<int>(tags->track()));
>          }

Are you sure we need this cast ? I do not think we will ever need to have negative track numbers added. The original type is unsigned int and should exactly convey the fact that we do not expect negative numbers.

> taglibextractor.cpp:393
>          if (tags->year()) {
> -            result->add(Property::ReleaseYear, tags->year());
> +            result->add(Property::ReleaseYear, static_cast<int>(tags->year()));
>          }

idem

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D11365

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, ngraham, alexeymin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180328/3f9b339b/attachment-0001.html>