celix
celix copied to clipboard
Feature/674 improve properties
This PR add support for setting and getting string, bool, long, double and a version array list with properties.
Also:
- Add support for only getting a property value if it is stored with a specific type (getLong, getDouble, etc)
- Refactor filter to use the underlying property type when matching. This prevent the needed for memory allocation during filter match. The changed behavior should also more align with Properites and Filter matching in OSGi
- Add logic so that a service registration will use the correct type for service.id, service.bundleid and service.version and if needed convert service.ranking and service.version properties (if set before a service registration call) to the correct types.
- Add Properties / Filter intro documentation
- Improve test coverage for Filter and Properties
Refactoring the Properties store and load to include properties value type information will be done in a follow up PR.
~~I noticed Codecov has stopped working for 3 days in a row.~~ Fixed.
Codecov Report
Attention: Patch coverage is 99.33244% with 5 lines in your changes are missing coverage. Please review.
Project coverage is 90.17%. Comparing base (
8acbe78) to head (6a9713e).
Additional details and impacted files
@@ Coverage Diff @@
## master #721 +/- ##
==========================================
+ Coverage 89.84% 90.17% +0.32%
==========================================
Files 222 223 +1
Lines 26058 26545 +487
==========================================
+ Hits 23413 23937 +524
+ Misses 2645 2608 -37
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I'll finish reviewing this PR ASAP. Please help review #699.
The first round review finished. I noticed code duplications in
celix_properties, and thus suggest an alternative design:
- Add only one type to
celix_properties, i.e. ArrayList- Add only four (or more?) methods
setArrayList/assignArrayList/getArrayList/getAsArrayList- Move element type information into
celix_array_list, so that we can have the right copy operation for them.This way:
- We don't need to check arrayList's element types, which is totally left to arrayList
- The dangling pointer issue is solved naturally by the right copy operation in arrayList
- The code duplication in
celix_propertiesis eliminated.WDYT?
Sounds good and I will look into this. I am bit uncertain how this impact celix_array_list and its current usage.
Currently the array list implementation has no notion a an element type and just provided access to a union element entry.
A logically way to specify the array list entry type is during creations, but this impact all usage of celix_arrayList_create and celix_arrayList_createWithOptions.
An other options could be to set the element type when entries are added, so a call to celix_arrayList_addLong will make the array list element type long and a followup call to celix_arrayList_setDouble or celix_arrayList_getDouble, etc will fail.
Or maybe mixed element types should be allowed, but I do no think this is a valid use case.
The first round review finished. I noticed code duplications in
celix_properties, and thus suggest an alternative design:
- Add only one type to
celix_properties, i.e. ArrayList- Add only four (or more?) methods
setArrayList/assignArrayList/getArrayList/getAsArrayList- Move element type information into
celix_array_list, so that we can have the right copy operation for them.This way:
- We don't need to check arrayList's element types, which is totally left to arrayList
- The dangling pointer issue is solved naturally by the right copy operation in arrayList
- The code duplication in
celix_propertiesis eliminated.WDYT?
Sounds good and I will look into this. I am bit uncertain how this impact celix_array_list and its current usage.
Currently the array list implementation has no notion a an element type and just provided access to a union element entry. A logically way to specify the array list entry type is during creations, but this impact all usage of
celix_arrayList_createandcelix_arrayList_createWithOptions.An other options could be to set the element type when entries are added, so a call to
celix_arrayList_addLongwill make the array list element type long and a followup call tocelix_arrayList_setDoubleorcelix_arrayList_getDouble, etc will fail.Or maybe mixed element types should be allowed, but I do no think this is a valid use case.
I will create a separate pull request to add element type support to the array list, including a array list copy function. And when the array list with element type pull request is merged, I will update this PR and simplify the typed array list support for properties.
This PR has been updated so that celix array list types are used, which minimizes the celix properties functions needed for array list handling. This PR can be reviewed again.
After this, I will dive into properties (JSON) serialization.
All issues have been addressed! :rocket: Please wait for a little last minute cleanup to be merged, which will happen tomorrow.