#98 Improve dbscan output
Closed: Fixed 6 years ago Opened 6 years ago by sramling.

The current implementation of "dbscan" function produces output with \n, \t and \x characters.


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

6 years ago

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.

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.

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!!!

Metadata Update from @sramling:
- Custom field Review Status adjusted to review (was: None)

6 years ago

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.

You can do. So we won't have two unnecessary lines.

list.extend(['-s', suffix])

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.

Reviewed your patch. Looks fine.

Metadata Update from @sramling:
- Custom field Review Status adjusted to ack (was: review)

6 years ago

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)

6 years ago

Login to comment on this ticket.

Metadata