-
Notifications
You must be signed in to change notification settings - Fork 247
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
Small refactor to metrics #33
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.
Some small changes needed.
norfair/metrics.py
Outdated
value_string += self.file[index_position_on_this_document] | ||
index_position_on_this_document += 1 | ||
return int(value_string) | ||
|
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.
This empty line shouldn't be here.
norfair/metrics.py
Outdated
while self.file[line][0 : len(variable_name)] != variable_name: | ||
line += 1 |
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 catch to the exception that will get raised if we don't find the variable_name with a descriptive error message. As it is currently I think we'll raise a generic index error as the line
variable will eventually be out of bounds of we don't find the variable_name.
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 actually write this class like this:
class InformationFile:
def __init__(self, file_path):
self.path = file_path
with open(file_path, "r") as myfile:
self.file = myfile.read()
self.lines = self.file.splitlines()
def search(self, variable_name):
for line in self.lines:
if line[: len(variable_name)] == variable_name:
result = line[len(variable_name) + 1 :]
break
else:
raise ValueError(f"Couldn't find '{variable_name}' in {self.path}")
if result.isdigit():
return int(result)
else:
return result
Please contact me when you reach this comment as there are several very cool python things to talk about regarding this. Just as a remind for myself those things are (Exception vs if, for else statements, string's index method and fstrings).
Add a descriptive error statement for the class InformationFile
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.
This is great, two small changes and its ready for merging!
norfair/video.py
Outdated
self.imExt = information_file.search("imExt") | ||
self.imDir = information_file.search("imDir") |
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.
Python code style states that objects should be named using 'lower_case_with_underscores' and classes should be named using 'CamelCase'. Also, it is better to use more descriptive names, unless they are way too long, which is not the case here.
Therefore, if I am understanding what these variables are correctly, I would rename them to self.image_extension
and self.image_directory
.
norfair/metrics.py
Outdated
@@ -10,21 +10,22 @@ | |||
|
|||
class InformationFile: | |||
def __init__(self, file_path): | |||
self.path = file_path | |||
with open(file_path, "r") as myfile: | |||
self.file = myfile.read() |
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.
self.file
is only used here, so we can remove it from self
and just name it file
.
Also, 'file' is no longer an attribute in InformationFile class.
Perfect, thanks! |
No description provided.