bruns added a comment.
In D16490#352199 <https://phabricator.kde.org/D16490#352199>, @astippich wrote: > In D16490#352109 <https://phabricator.kde.org/D16490#352109>, @bruns wrote: > > > In D16490#351935 <https://phabricator.kde.org/D16490#351935>, @astippich wrote: > > > > > In D16490#351799 <https://phabricator.kde.org/D16490#351799>, @bruns wrote: > > > > > > > In D16490#351662 <https://phabricator.kde.org/D16490#351662>, @astippich wrote: > > > > > > > > > Only one minor thing: please also check that the mimetype is in the list of supported mimetypes > > > > > > > > > > > > This can actually happen and is completely valid, due to mimetype inheritance. > > > > > > > > So the check would be `for supported in supportedMimetypes { if QMimeType(input->mimeType()).inherits(supported) return true; }; return false`. But this is already done from the calling code ... > > > > > > > > > Hmmm, I don't understand. When I change the code to return an empty stringlist of supported mimetypes for the xmlextractor, the tests still pass. > > > This should imho be covered by the tests. > > > > > > This is one level above these tests. The surrounding code ensures the right extractor is called for each file, see `ExtractorCollection::fetchExtractors(...)`. > > > Right, and if e.g. the list of supported mimetypes is empty, the corresponding extractor will never be selected because ExtractorCollection doesn't know that the mimetype is supported by this extractor. > Hence we should ensure and test imho that the list of supported mimetypes provided to the ExtractorCollection is correct for this extractor. I'm not calling for testing that the right extractor is selected. The unit tests do not use ExtractorCollection, because they test the extractors, not ExtractorCollection. The extractor unit tests explicitly pass the mime type to the extractor. We don't want to double the checks. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16490 To: bruns, #frameworks, astippich Cc: lbeltrame, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
