Review of T19791; PropertyServices should also handle wild cards...
Summary:
In T19791#70153, @floca wrote:First of all. Thanks for crawling through the patch!
Do you have some points that you're not sure/confident about?
Nope. I am fine. Everything that was sure we had already discussed at our hack fest beforehand.
Did you already let different nightlies build your branch?
Not yet. I waited till I have the human review, because after that, I have to go through the nightlies anyway.
Is the property view still working as expected as you changed property services which the view is using?
Yes. I tested it. And at last all features I know of, seem to work fine.
Typos.
Thanks. Fixed.
Important stuff
Copyright notice for CreateJSONEscapes()
It is based on code from boost property tree. Regarding the boost license, formaly there is no need for a copy right notice. But I think adding a comment on that is a good idea.
Considerations
- Infos -> Info?
Fixed.
- std::unordered_multimap instead of std::multimap
In context of PropertyPersistence::InfoMap I indeed want to keept the ordering. ;)
[...] I would prefer the direct type in the following example as it is often more clear for the client of what he can expect as return value: using MimeTypeNameType = std::string
Here I disagree (But it is a matter of taste and maybe I am so template adapted that it does not annoy me enough). I would keep it because I want to emphasize the dependency on the type defined by PeropertyPersistenceInfo
PropertyPersistenceInfo is already a strong dependency for the IPropertyPersistence class interface. So we would gain no benefit in terms of class indepence from decoupling this type only.Why the extra namespaces ...
I am indifferent too; same reasoning. I would keep it like it currently is. If there is a clear design descision at some point, we can refactor it.
Split inline lambdas to multiple lines instead of single line monsters
Done.
fixed typos and documentation (review feedback)
Signed-off-by: Ralf Floca <r.floca@dkfz-heidelberg.de>
made lambda more readable (review feedback); fixed assert bug in PropertyPersistenceInfo
Signed-off-by: Ralf Floca <r.floca@dkfz-heidelberg.de>
refactored method naming in IPropertyPersistence(review feedback)
Signed-off-by: Ralf Floca <r.floca@dkfz-heidelberg.de>
added missing header to avoid compile errors for linux
Signed-off-by: Ralf Floca <r.floca@dkfz-heidelberg.de>
Merge branch 'bug-19791-PropertyServices_with_wildcards' into T19791-Integration-Branch
- Conflicts:
- Modules/Core/src/DataManagement/mitkTemporoSpatialStringProperty.cpp
- Modules/Core/test/mitkPropertyPersistenceTest.cpp
Signed-off-by: Ralf Floca <r.floca@dkfz-heidelberg.de>
fixed error in DiffusionCore due to changed IPropertyPersistence interface.
Signed-off-by: Ralf Floca <r.floca@dkfz-heidelberg.de>
Test Plan: 1) Run unit tests.
Reviewers: O1 MITK Core, goch
Reviewed By: O1 MITK Core, goch
Subscribers: goch, kislinsk
Maniphest Tasks: T19791
Differential Revision: https://phabricator.mitk.org/D8