-
Notifications
You must be signed in to change notification settings - Fork 36
Conversation
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
+ Coverage 66.22% 66.77% +0.54%
==========================================
Files 5 6 +1
Lines 1448 1496 +48
==========================================
+ Hits 959 999 +40
- Misses 489 497 +8
Continue to review full report at Codecov.
|
58aa144
to
d74b6e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor hints from my side 👍
ospd/misc.py
Outdated
@@ -852,7 +852,7 @@ def main(name, klass): | |||
cargs = get_common_args(parser) | |||
logging.getLogger().setLevel(cargs['log_level']) | |||
wrapper = klass(certfile=cargs['certfile'], keyfile=cargs['keyfile'], | |||
cafile=cargs['cafile']) | |||
cafile=cargs['cafile'], customvtfilter=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required. It's an optional keyword that defaults to None.
cafile=cargs['cafile'], customvtfilter=None) | |
cafile=cargs['cafile']) |
|
||
_vts_aux = vts.copy() | ||
for _element, _oper, _filter_val in filters: | ||
for vt_id in _vts_aux.copy(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is not necessary to create another copy by using? Not sure.
for vt_id in _vts_aux.copy(): | |
for vt_id in vts: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy is need since the _vts_aux dictionary is change during the iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but _vts_aux is already a copy of vts. Therefore IMHO it should be possible to iterate over the original unchanged vts variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment you can pass more than one filter option separated by a semicolon. E.g. :
<get_vts filter='modification_time>201903130813;modification_time<201903130815'/>
The second one must be applied over the result of the first one (logical AND operation).
If vts is used instead, the logical operation will be an OR, and the result will be the whole vts collection.
ospd/vtfilter.py
Outdated
format_func = self.allowed_filter.get(element) | ||
return format_func(value) | ||
|
||
def get_filtered_vts_list(self, vts, vt_filter=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not use a optional keyword argument for a required parameter. instead
def get_filtered_vts_list(self, vts, vt_filter=None): | |
def get_filtered_vts_list(self, vts, vt_filter): |
Besides that the vts
argument isn't documented.
@bjoernricks Thanks a lot for the review! |
It is possible to get a specific collection of VTS with the filter.
Use:
<get_vts filter='modification_time>20190320'/>