Added some utility in disk library #3637
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3637 +/- ##
==========================================
- Coverage 69.38% 69.35% -0.03%
==========================================
Files 133 133
Lines 17015 17029 +14
==========================================
+ Hits 11806 11811 +5
- Misses 5209 5218 +9
Continue to review full report at Codecov.
|
PraveenPenguin
force-pushed
the
disk_utils
branch
from
2e600a4
to
e02b7c8
Compare
added a utility that performs the operation on io scheduler Signed-off-by: Praveen K Pandey <praveen@linux.vnet.ibm.com>
PraveenPenguin
force-pushed
the
disk_utils
branch
from
e02b7c8
to
476b3e5
Compare
There was a problem hiding this comment.
Hi @PraveenPenguin, so far, your code looks promising. I have made some comments, but I also would like you to check double spaces all over your docstrings. I have not added individual comments to those as it would pollute the review.
| :return: list of IO scheduler | ||
| """ | ||
| names = open(__sched_path(device_name)).read() | ||
| return names.translate(string.maketrans('[]', ' ')).split() |
There was a problem hiding this comment.
What python version are you running? I tried this code with python 3.7 and got AttributeError: module 'string' has no attribute 'maketrans'. I also found this note on python release 3.1: The string.maketrans() function is deprecated and is replaced by new static methods, bytes.maketrans() and bytearray.maketrans().
There was a problem hiding this comment.
ah let me check my setup
|
|
||
| from . import process | ||
|
|
||
|
|
||
| class DiskUtilsError(Exception): |
There was a problem hiding this comment.
This is an excellent intention, but I suggest you use a more specific exception, like OSError, for example.
There was a problem hiding this comment.
@beraldoleal, I agree, but it, at least, covers a more specific set of exceptions.
There was a problem hiding this comment.
@willianrampazzo idea here is it catch Exceptions for utility, right now it seems OSError but idea is that Exceptions can be reused for any exception from this utility as I am planning to add more library will use this, please let me your thought
There was a problem hiding this comment.
IMO, DiskUtilsError is better than an OSError, because tells the user that the error was in our library. But, I might be missing something.
There was a problem hiding this comment.
@beraldoleal I also agree with this @willianrampazzo please let us know your thought so we can have V2
There was a problem hiding this comment.
My initial comment was about changing class DiskUtilsError(Exception) by class DiskUtilsError(OSError) as OSError is more specific to what is been covered here, and not changing the DiskUtilsError by OSError.
But I don't have problems keeping the way it is now, it was just a suggestion to restrict the kind of exceptions covered by DiskUtilsError.
There was a problem hiding this comment.
@willianrampazzo sure but I feel we need to have DiskUtilsError having broader Exception so we need not to change it on future but when we raise it in utility we can narrow down there like in OSError case we have to expect in that utility how that sound
There was a problem hiding this comment.
Okay, I got it. Let's go with Exception.
| :param device_name: Device name example like sda , hda | ||
| :return: list of IO scheduler | ||
| """ | ||
| names = open(__sched_path(device_name)).read() |
There was a problem hiding this comment.
You are using context manager all over the code, so just for the sake of consistency, my suggestion is to use a context manager here too.
There was a problem hiding this comment.
sure make sense
| :rtype : str | ||
| """ | ||
| return re.split(r'[\[\]]', | ||
| open(__sched_path(device_name)).read())[1] |
There was a problem hiding this comment.
Same comment for the context manager here.
|
@PraveenPenguin any updates here? Do you still need this or can we close? |
@beraldoleal ah i miss this will send PR soon |
richtja
force-pushed
the
master
branch
12 times, most recently
from
6a7bd06
to
07e4ea2
Compare
Hi @PraveenPenguin, Gentle ping here. |
|
Hi @PraveenPenguin do you plan to follow up on this PR? Thank you for your answer. |
Hi @richtja , ah missed this PR my bad , will respin adopting with review comment asap , |
|
Hi @PraveenPenguin gentle ping here. |
|
closing this PR as raised new version of this code changes #5928 |
added a utility that performs the operation on io scheduler
Signed-off-by: Praveen K Pandey praveen@linux.vnet.ibm.com