Fix more PEP 8 findings and warnings #723
Conversation
|
|
||
| __appname__ = 'labelImg' | ||
|
|
||
|
|
||
| class WindowMixin(object): |
There was a problem hiding this comment.
Rationale: I integrated the WindowMixin into the main window class since this seemed the simpler solution. If you had a specific purpose in mind for it, let me know
| menu = self.menuBar().addMenu(title) | ||
| if actions: | ||
| add_actions(menu, actions) | ||
| return menu | ||
|
|
||
| def toolbar(self, title, actions=None): | ||
| def add_toolbar(self, title, actions=None): |
There was a problem hiding this comment.
Rationale: I renamed the function to make it consistent with add_menu
| @@ -83,9 +82,14 @@ def __init__(self, default_filename=None, default_prefdef_class_file=None, defau | |||
| self.settings.load() | |||
| settings = self.settings | |||
|
|
|||
| self.image_data = None | |||
There was a problem hiding this comment.
Rationale: members should be initialized in the init-function
| # Load string bundle for i18n | ||
| self.string_bundle = StringBundle.get_bundle() | ||
| get_str = lambda str_id: self.string_bundle.get_string(str_id) |
There was a problem hiding this comment.
Rationale: Fix warning that lambas should not be assigned, instead define a function
| @@ -204,141 +206,145 @@ def __init__(self, default_filename=None, default_prefdef_class_file=None, defau | |||
|
|
|||
| # Actions | |||
| action = partial(new_action, self) | |||
| quit = action(get_str('quit'), self.close, | |||
There was a problem hiding this comment.
Rationale: Avoid using reserved names as variables.
For consistency I prepended "action_" to all action variables
|
|
||
| def get_format_meta(format): |
There was a problem hiding this comment.
Rationale: Avoid reserved names for variables
| @@ -350,33 +356,57 @@ def get_format_meta(format): | |||
| self.draw_squares_option.setChecked(settings.get(SETTING_DRAW_SQUARE, False)) | |||
| self.draw_squares_option.triggered.connect(self.toggle_draw_square) | |||
|
|
|||
| # Store actions for further handling. | |||
There was a problem hiding this comment.
This change might be a bit controversial.
Rationale: The Struct class injects members into a class in a way that linters cannot resolve, therefore they produce a lot of warnings. The does not seem to be a really good alternative to the Struct class after some research, so I opted to simply write a normal class manually and fill it with the actions.
I would suggest to eventually replace all this setup stuff by using the QtDesigner as it will generate the code and therefore significantly reduce the amount of manual code to be reviewed.
| @@ -447,7 +478,7 @@ def get_format_meta(format): | |||
| recent_file_qstring_list = settings.get(SETTING_RECENT_FILES) | |||
| self.recent_files = [ustr(i) for i in recent_file_qstring_list] | |||
| else: | |||
| self.recent_files = recent_file_qstring_list = settings.get(SETTING_RECENT_FILES) | |||
There was a problem hiding this comment.
Rationale: removed unused variable
| @@ -723,7 +756,7 @@ def file_item_double_clicked(self, item=None): | |||
| self.load_file(filename) | |||
|
|
|||
| # Add chris | |||
| def button_state(self, item=None): | |||
There was a problem hiding this comment.
Rationale: removed unused function argument
|
|
||
| try: | ||
| shape = self.items_to_shapes[item] | ||
| except: |
There was a problem hiding this comment.
Rationale: The second try/catch does not catch anything that the first did not catch anyway, so I integrated them into a single try/except block
| @@ -849,21 +878,18 @@ def format_shape(s): | |||
| if self.label_file_format == LabelFileFormat.PASCAL_VOC: | |||
| if annotation_file_path[-4:].lower() != ".xml": | |||
| annotation_file_path += XML_EXT | |||
| self.label_file.save_pascal_voc_format(annotation_file_path, shapes, self.file_path, self.image_data, | |||
| self.line_color.getRgb(), self.fill_color.getRgb()) | |||
| self.label_file.save_pascal_voc_format(annotation_file_path, shapes, self.file_path, self.image_data) | |||
There was a problem hiding this comment.
Rationale: The line and fill colors are not supported by the voc format, so I removed the call here. The same goes for the yolo and createML format
Having these arguments here may be a preparation for a future change to the code, but I followed the YAGNI principle here
| elif self.label_file_format == LabelFileFormat.CREATE_ML: | ||
| if annotation_file_path[-5:].lower() != ".json": | ||
| annotation_file_path += JSON_EXT | ||
| self.label_file.save_create_ml_format(annotation_file_path, shapes, self.file_path, self.image_data, |
There was a problem hiding this comment.
Finding: The CreateML format is currently not supported in the tool. It can be selected, but saving and loading does not work. I assume that this is a temporary fix for some incompatibility, but I would suggest either fixing it or getting rid of the format altogether to not have unexpected behavior in the tool.
I added an issue for this: #724
| @@ -1007,19 +1033,19 @@ def zoom_request(self, delta): | |||
| new_h_bar_value = h_bar.value() + move_x * d_h_bar_max | |||
| new_v_bar_value = v_bar.value() + move_y * d_v_bar_max | |||
|
|
|||
| h_bar.setValue(new_h_bar_value) | |||
| v_bar.setValue(new_v_bar_value) | |||
| h_bar.setValue(int(new_h_bar_value)) | |||
There was a problem hiding this comment.
Rationale: There was an implicit conversion from float to int.
I changed it so that this is now explicit.
Both the implicit and the explicit conversions round down. If you want to have proper rounding up at 0.5, this needs to be changed.
To do:
- Review and change if necessary
| @@ -1053,7 +1079,7 @@ def load_file(self, file_path=None): | |||
| if unicode_file_path and os.path.exists(unicode_file_path): | |||
| if LabelFile.is_label_file(unicode_file_path): | |||
| try: | |||
| self.label_file = LabelFile(unicode_file_path) | |||
There was a problem hiding this comment.
Rationale: Removed unused constructor argument. Seems like an older artifact from a past refactoring step
| @@ -1102,7 +1128,7 @@ def load_file(self, file_path=None): | |||
| self.label_list.setCurrentItem(self.label_list.item(self.label_list.count() - 1)) | |||
| self.label_list.item(self.label_list.count() - 1).setSelected(True) | |||
|
|
|||
| self.canvas.setFocus(True) | |||
There was a problem hiding this comment.
Finding: The setFocus function expects an enum, not a bool.
I looked up the corresponding value to True == 1 == Qt.TabFocusReason, so the behavior should be unchanged.
However, you might want to check out whether this does what you want it to do.
To do:
- check if the enum value is what's intended here.
| @@ -1250,16 +1277,15 @@ def open_annotation_dialog(self, _value=False): | |||
| filename = filename[0] | |||
| self.load_pascal_xml_by_filename(filename) | |||
|
|
|||
| def open_dir_dialog(self, _value=False, dir_path=None, silent=False): | |||
There was a problem hiding this comment.
Rationale: Remove unused argument
| if not self.may_continue(): | ||
| return | ||
|
|
||
| default_open_dir_path = dir_path if dir_path else '.' |
There was a problem hiding this comment.
Rationale: This variable is overwritten in all following paths so I removed it
| @@ -1573,23 +1600,25 @@ def read(filename, default=None): | |||
| return default | |||
|
|
|||
|
|
|||
| def get_main_app(argv=[]): | |||
| def get_main_app(argv=None): | |||
There was a problem hiding this comment.
Rationale: Function arguments should not be mutable.
| @@ -395,8 +395,6 @@ def bounded_move_vertex(self, pos): | |||
|
|
|||
| left_index = (index + 1) % 4 | |||
| right_index = (index + 3) % 4 | |||
| left_shift = None | |||
There was a problem hiding this comment.
Rationale: Variables are overwritten in all following paths, so they can be removed
| @@ -503,8 +501,8 @@ def paintEvent(self, event): | |||
|
|
|||
| if self.drawing() and not self.prev_point.isNull() and not self.out_of_pixmap(self.prev_point): | |||
| p.setPen(QColor(0, 0, 0)) | |||
| p.drawLine(self.prev_point.x(), 0, self.prev_point.x(), self.pixmap.height()) | |||
| p.drawLine(0, self.prev_point.y(), self.pixmap.width(), self.prev_point.y()) | |||
| p.drawLine(int(self.prev_point.x()), 0, int(self.prev_point.x()), int(self.pixmap.height())) | |||
There was a problem hiding this comment.
Rationale: There was an implicit conversion from float to int.
I changed it so that this is now explicit.
Both the implicit and the explicit conversions round down. If you want to have proper rounding up at 0.5, this needs to be changed.
To do:
- Review and change if necessary
| @@ -13,9 +13,12 @@ | |||
|
|
|||
|
|
|||
| class ComboBox(QWidget): | |||
| def __init__(self, parent=None, items=[]): | |||
| def __init__(self, parent=None, items=None): | |||
There was a problem hiding this comment.
Rationale: Function arguments should not be mutable
| @@ -32,13 +30,13 @@ class LabelFile(object): | |||
| # suffix = '.lif' | |||
| suffix = XML_EXT | |||
|
|
|||
| def __init__(self, filename=None): | |||
There was a problem hiding this comment.
Rationale: Removed unused function argument
| self.shapes = () | ||
| self.image_path = None | ||
| self.image_data = None | ||
| self.verified = False | ||
|
|
||
| def save_create_ml_format(self, filename, shapes, image_path, image_data, class_list, line_color=None, fill_color=None, database_src=None): |
There was a problem hiding this comment.
Rationale: Removed unused function arguments (also for yolo and pascal formats)
This might be a preparation for new functionality or an artifact from old code. Since you can choose box colors, but are unable to save them, I assume it's from old code and I think it makes sense to decide to either drop the box color support althogether or re-introduce saving the colors to the output files.
Following the YAGNI principle, I removed this. If you want to keep it, I can revert the change. I would suggest adding an issue about saving box colors in that case so it is not forgotten.
| @@ -78,9 +79,13 @@ def gen_xml(self): | |||
| return top | |||
|
|
|||
| def add_bnd_box(self, x_min, y_min, x_max, y_max, name, difficult): | |||
| bnd_box = {'xmin': x_min, 'ymin': y_min, 'xmax': x_max, 'ymax': y_max} | |||
There was a problem hiding this comment.
Rationale: Unified the definition into a single dictionary creation call
| @@ -112,7 +117,6 @@ def append_objects(self, top): | |||
| def save(self, target_file=None): | |||
| root = self.gen_xml() | |||
| self.append_objects(root) | |||
| out_file = None | |||
There was a problem hiding this comment.
Rationale: Variable is overwritten in all paths, so it can be removed
| @@ -152,7 +156,6 @@ def parse_xml(self): | |||
| assert self.file_path.endswith(XML_EXT), "Unsupported file format" | |||
| parser = etree.XMLParser(encoding=ENCODE_METHOD) | |||
| xml_tree = ElementTree.parse(self.file_path, parser=parser).getroot() | |||
| filename = xml_tree.find('filename').text | |||
There was a problem hiding this comment.
Rationale: Variable is never used. Not sure whether this is a code artifact or points to a bug. May make sense to look into it
To do:
- check if this is correct behavior
| key = key_value[0].strip() | ||
| value = PROP_SEPERATOR.join(key_value[1:]).strip().strip('"') | ||
| self.id_to_message[key] = value | ||
| while not text.atEnd(): |
There was a problem hiding this comment.
Rationale: This would have lead to a crash if f.open() failed.
| @@ -62,12 +62,6 @@ def label_validator(): | |||
| return QRegExpValidator(QRegExp(r'^[^ \t].+'), None) | |||
|
|
|||
|
|
|||
| class Struct(object): | |||
There was a problem hiding this comment.
| def have_qstring(): | ||
| """p3/qt5 get rid of QString wrapper as py3 has native unicode str type""" | ||
| return not (sys.version_info.major >= 3 or QT_VERSION_STR.startswith('5.')) | ||
|
|
||
| def util_qt_strlistclass(): |
There was a problem hiding this comment.
Rationale: Remove unused function
|
|
||
| def natural_sort(list, key=lambda s:s): |
There was a problem hiding this comment.
Rationale: Avoid using reserved names in variables
| """ | ||
| Sort the list into natural alphanumeric order. | ||
| """ | ||
| def get_alphanum_key_func(key): |
There was a problem hiding this comment.
Rationale: Avoid using reserved names in variables
| return lambda s: [convert(c) for c in re.split('([0-9]+)', key(s))] | ||
| def get_alphanum_key_func(internal_key): | ||
|
|
||
| def convert(text): |
There was a problem hiding this comment.
Rationale: Avoid assigning a lambda expression, prefer using a function instead
See #725 for general details.
I added comments to explain why I changed something.
For ease of review please use the "Files changed" tab.