The current implementation of "dbscan" function produces output with \n, \t and \x characters.
Adding a patch to change the output of dbscan.
<img alt="0001-fix-dbscan-out.patch" src="/lib389/issue/raw/files/b240188a582e4d6c094e3eea30cef4b028fb2420f4876ae55e91f0b2a438c42d-0001-fix-dbscan-out.patch" />
Commit message updated.
<img alt="0001-fix-dbscan-out.patch" src="/lib389/issue/raw/files/8f18fbd0435bb2f48068c99ffe1a4f33822b587cdd1007d99c384625bcca98da-0001-fix-dbscan-out.patch" />
Probably, if you don't mind, we need to write a proper docstring, so we can add it to the sphinx docs. For this, please, refer - https://pagure.io/lib389/blob/master/f/lib389/repltools.py#_250
Also, I think, if you've decided to modify the function this way, we need to make it properly. Instead of using 'shell=True', we better to pass a list to the command part. Like ['dbscan', '-n', bename, '-s', suffix, etc.]. We can construct it with 'list.append' method.
I am not sure why you moved the logging to try-except block. We catch OSError in the try-except and the error will not happen with logging. So there is no point in it.
Metadata Update from @spichugi: - Custom field Origin adjusted to None - Custom field Review Status adjusted to None
Thanks for your suggestion. I made changes to the command line arguments in a list and appended them. Regarding the change for docstring, I will make a change later as I am trying to finish of other automation tasks in replication.
<img alt="0001-fix-dbscan-out.patch" src="/lib389/issue/raw/files/9c229c11f6c5674fe6af650fddab5df94d346aff528ee0e932311cddfd73c249-0001-fix-dbscan-out.patch" />
Fixed the commit message
<img alt="0001-fix-dbscan-out.patch" src="/lib389/issue/raw/files/8e0ef32b53c7cb775d1a663a7c6e8669c908c296cbf8489680d64acf7d66f435-0001-fix-dbscan-out.patch" />
Please, put it in 1 line instead of 4. It is not a long command or something.
24 + cmd = [ 25 + prog, 26 + '-f', indexfile 27 + ]
You can use 'list.extend' in this case. If you want to append the list, you use 'list.extend'. The info is available in standard library docs.
42 + cmd.append('-t') 43 + cmd.append(width)
And for docs:
Regarding the change for docstring, I will make a change later as I am trying to finish of other automation tasks in replication.
It is a five minutes work, I don't see the reason to split it in two commits. You just need to have first line with brief description and to replace @param with :param: and to add :type: for each. (also mention what it :returns: and :raises:)
I am not sure, I understand your problem. If my understanding is correct, to extend a list, I need make a list and then extend it. If its a string, say '-k', then it appends like '-' and 'k'. I am not an expert, but I tried from my local tests and it didn't work as expected.
I totally agree. I need to collect the exact data, make a change and wait for that to be reviewed. I am sure you will have more comments on that. I will do that once I am done with my deadline tasks. Thanks!!!
Fixed the list in one line.
<img alt="0001-fix-dbscan-out.patch" src="/lib389/issue/raw/files/b8852d368512f2bf5fcfb5912802bf8d18d2496e046bd12d29931495d9c69272-0001-fix-dbscan-out.patch" />
Metadata Update from @sramling: - Custom field Review Status adjusted to review (was: None)
You can do. So we won't have two unnecessary lines.
list.extend(['-s', suffix])
Adding a new patch.
<img alt="0001-fix-dbscan-out.patch" src="/lib389/issue/raw/files/bd77cfa7416a20e48ce1e1e62c163f229e06c879c58654dc8abd11b76a60530b-0001-fix-dbscan-out.patch" />
It is a five minutes work, I don't see the reason to split it in two commits. You just need to have first line with brief description and to replace @param with :param: and to add :type: for each. (also mention what it :returns: and :raises:) I totally agree. I need to collect the exact data, make a change and wait for that to be reviewed. I am sure you will have more comments on that. I will do that once I am done with my deadline tasks. Thanks!!!
Ok, I understand that you are under the stress. I'll write the docstring. Please, check if you think it is correct.
<img alt="0001-Issue-98-Fix-dbscan-output.patch" src="/lib389/issue/raw/files/b2b6737c13e4d98f7b63831b137c53ceaf60105bf4621859eb29cc23b1cc2848-0001-Issue-98-Fix-dbscan-output.patch" />
Reviewed your patch. Looks fine.
Metadata Update from @sramling: - Custom field Review Status adjusted to ack (was: review)
commit 121efec Author: Sankar Ramalingam sramling@redhat.com Date: Fri Sep 22 23:27:54 2017 +0530
Metadata Update from @spichugi: - Issue close_status updated to: Fixed - Issue status updated to: Closed (was: Open)
Login to comment on this ticket.