Feature/separate interfaces #5626
Conversation
|
Very nice. Is this enough to completely hide Xerces from the interface or is there more to be done? The change in loading might break a lot of pyopenms workflows but in my opinion it is worth it. We probably should do an increase in the major version then. |
there is more to be done, this is just a first step for the worst offenders: basically we would have to do one of two things for all of these: write wrapper functions in FileHandler or separate implementation (with XMLHandler) from interface. Quite a bit of work but it should also help with #5619 and make sure that downstream consumers dont pull in stuff they dont need/want.
as long as we keep the old interfaces around, this should not actually be a problem |
| @@ -1039,8 +1039,7 @@ namespace OpenMS | |||
| FeatureMapType fm_missing; | |||
| cl.suggestMissingFeatures(fm_out, cons_map, fm_missing); | |||
|
|
|||
| FeatureXMLFile fmf; | |||
| fmf.store("fm_missing.featureXML", fm_missing); | |||
| FileHandler.storeFeatures("fm_missing.featureXML", fm_missing); | |||
There was a problem hiding this comment.
maybe worth to dicuss: is it a better API for a user to have this moved to a FileHandler class? Wouldn't it be more natural to have such a method in FeatureMap? Separation is still be possible if I am not mistaken.
There was a problem hiding this comment.
e.g., keeping the old interface with FeatureXMLFile, so code doesn't break and call some internal FeatureXMLFileHandler
There was a problem hiding this comment.
I dont think we should intertwine the data structure (FeatureMap) with the implementation (XML), especially as we may store the data in a different format in the future, e.g. SQL or other formats. I think the correct way to do it is have an interface along the lines of StoreAsXML and potentially StoreAsXXX in the future
There was a problem hiding this comment.
e.g., keeping the old interface with FeatureXMLFile, so code doesn't break and call some internal FeatureXMLFileHandler
this is what we do for MzMLFile and MzMLFileHandler and what I implemented here in this PR as well, separating the interface FeatureXMLFile and FeatureXMLFileHandler. But the question that makes sense is whether we need 2 interfaces, FileHandler and MzMLFile
There was a problem hiding this comment.
Ok I see. I agree - having a FeatureXMLFile interface looks a bit cleaner designwise compared to having only a "FileHandler" class. But no strong opinion here.
There was a problem hiding this comment.
But let's keep the two interfaces for now
|
@timosachsenberg per cdash all tests pass and builds work: http://cdash.openms.de/index.php?project=OpenMS&date=2021-10-29 should we merge? |
|
rebuild jenkins |
|
Thanks! |
Description
This is a subset of the changes from #5613 that I made when trying to separate OpenMS into different libraries. The main thing this PR does is to remove the dependency of various parts of OpenMS on xercesc which gets pulled in with
XMLHandler. Unfortunately, includingXMLHandler.his quite common in OpenMS which leads to a large number of cross-dependencies and prevents us from using clean interfaces to interact with files.One part of this PR is to introduce a new interface for FeatureXML and ConsensusXML and move the implementation into the
OpenMS::InternalnamespaceSecondly, it uses the
FileHandleras a clean interface to load and write filesChecklist: