-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support to process empty records #402
Conversation
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 have some additional suggestions - we talked about adding tests, see below.
More intuitive name
I think allow_empty_list
probably doesn't mean anything to our users so maybe something more along the lines of allow_empty_chunks
or allow_empty_records
Generating commands
I was also thinking about generating commands, they have their own implementation for _execute_chunk_v2 that obviously assumes that no events are present because that's how generating commands work. So we should probably enforce that by not supplying to parameter in the process
method for generating commands, so adding this to https://github.com/splunk/splunk-sdk-python/blob/master/splunklib/searchcommands/generating_command.py (add adding import sys
:
def process(self, argv=sys.argv, ifile=sys.stdin, ofile=sys.stdout):
""" Process data.
:param argv: Command line arguments.
:type argv: list or tuple
:param ifile: Input data file.
:type ifile: file
:param ofile: Output data file.
:type ofile: file
:return: :const:`None`
:rtype: NoneType
"""
# Force allow_empty_list to be true since generating commands assume an empty input
return self.process(argv=argv, ifile=ifile, ofile=ofile, allow_empty_list=True)
Allow empty by default
I was also thinking about actually setting allow_empty_list
to True
by default for all commands since that seems to match our previous behavior.
Let me know what you think.
- Change name of flag to something more intuitive like
allow_empty_chunks
or `allow_empty_records - Add tests for
allow_empty_list
in https://github.com/splunk/splunk-sdk-python/blob/master/tests/searchcommands/test_search_command.py - Change logic to enforce that generating commands always allow empty input records
- Change to allow empty records by default instead of throwing an error (True instead of False)
@@ -1063,8 +1072,7 @@ def iteritems(self): | |||
SearchMetric = namedtuple('SearchMetric', ('elapsed_seconds', 'invocation_count', 'input_count', 'output_count')) | |||
|
|||
|
|||
|
|||
def dispatch(command_class, argv=sys.argv, input_file=sys.stdin, output_file=sys.stdout, module_name=None): | |||
def dispatch(command_class, argv=sys.argv, input_file=sys.stdin, output_file=sys.stdout, module_name=None, allow_empty_list=False): |
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.
Add a docstring below for the new arg
@@ -425,10 +426,16 @@ def process(self, argv=sys.argv, ifile=sys.stdin, ofile=sys.stdout): | |||
:param ofile: Output data file. | |||
:type ofile: file | |||
|
|||
:param allow_empty_list: Allow empty results |
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.
Slightly more descriptive:
Allow empty input records for the command, if False an Error will be returned if empty chunk body is encountered when read
7dac8ee
to
4b393b1
Compare
9e01faf
to
000fe6b
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.
Added suggestion for a unit test and code comment but overall this looks good - feel free to merge when those are added
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.
Thanks for all the updates, looks great 🚀
No description provided.