Skip to content
This repository has been archived by the owner on Feb 29, 2024. It is now read-only.

Fix more PEP 8 findings and warnings #723

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Cerno-b
Copy link
Contributor

@Cerno-b Cerno-b commented Mar 22, 2021

See #725 for general details.

I added comments to explain why I changed something.

For ease of review please use the "Files changed" tab.


__appname__ = 'labelImg'


class WindowMixin(object):
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: Avoid using reserved names as variables.
For consistency I prepended "action_" to all action variables


def get_format_meta(format):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: removed unused function argument


try:
shape = self.items_to_shapes[item]
except:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: Remove unused argument

if not self.may_continue():
return

default_open_dir_path = dir_path if dir_path else '.'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: Function arguments should not be mutable

@@ -32,13 +30,13 @@ class LabelFile(object):
# suffix = '.lif'
suffix = XML_EXT

def __init__(self, filename=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@Cerno-b Cerno-b Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: Remove unused function


def natural_sort(list, key=lambda s:s):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: Avoid using reserved names in variables

"""
Sort the list into natural alphanumeric order.
"""
def get_alphanum_key_func(key):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale: Avoid assigning a lambda expression, prefer using a function instead

@Cerno-b Cerno-b changed the title Fix warnings Fix more PEP 8 findings and warnings Mar 22, 2021
@Cerno-b Cerno-b mentioned this pull request Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant