Fix more PEP 8 findings and warnings by Cerno-b · Pull Request #723 · HumanSignal/labelImg

Cerno-b · 2021-03-22T06:27:52Z

See #725 for general details.

I added comments to explain why I changed something.

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

Cerno-b · 2021-03-22T06:29:24Z

 __appname__ = 'labelImg'


-class WindowMixin(object):


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

Cerno-b · 2021-03-22T06:31:34Z

        return menu

-    def toolbar(self, title, actions=None):
+    def add_toolbar(self, title, actions=None):


Rationale: I renamed the function to make it consistent with add_menu

Cerno-b · 2021-03-22T06:32:20Z

        self.settings.load()
        settings = self.settings

+        self.image_data = None


Rationale: members should be initialized in the init-function

Cerno-b · 2021-03-22T06:33:10Z

+
        # Load string bundle for i18n
        self.string_bundle = StringBundle.get_bundle()
-        get_str = lambda str_id: self.string_bundle.get_string(str_id)


Rationale: Fix warning that lambas should not be assigned, instead define a function

Cerno-b · 2021-03-22T06:34:52Z


        # Actions
        action = partial(new_action, self)
-        quit = action(get_str('quit'), self.close,


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

Cerno-b · 2021-03-22T06:35:15Z

+        action_save = action(get_str('save'), self.save_file,
+                             'Ctrl+S', 'save', get_str('saveDetail'), enabled=False)

-        def get_format_meta(format):


Rationale: Avoid reserved names for variables

Cerno-b · 2021-03-22T06:38:46Z

        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.


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.

Cerno-b · 2021-03-22T06:39:29Z

                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)


Rationale: removed unused variable

Cerno-b · 2021-03-22T06:40:04Z

                self.load_file(filename)

    # Add chris
-    def button_state(self, item=None):


Rationale: removed unused function argument

Cerno-b · 2021-03-22T06:40:44Z


        try:
            shape = self.items_to_shapes[item]
-        except:


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

Cerno-b · 2021-03-22T06:42:40Z

                    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)


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

Cerno-b · 2021-03-22T06:44:38Z

            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,


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

Cerno-b · 2021-03-22T06:49:28Z


-        h_bar.setValue(new_h_bar_value)
-        v_bar.setValue(new_v_bar_value)
+        h_bar.setValue(int(new_h_bar_value))


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

Cerno-b · 2021-03-22T06:51:12Z

        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)


Rationale: Removed unused constructor argument. Seems like an older artifact from a past refactoring step

Cerno-b · 2021-03-22T06:53:05Z

                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)


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.

Cerno-b · 2021-03-22T06:53:28Z

                    filename = filename[0]
            self.load_pascal_xml_by_filename(filename)

-    def open_dir_dialog(self, _value=False, dir_path=None, silent=False):


Rationale: Remove unused argument

Cerno-b · 2021-03-22T06:54:06Z

        if not self.may_continue():
            return

-        default_open_dir_path = dir_path if dir_path else '.'


Rationale: This variable is overwritten in all following paths so I removed it

Cerno-b · 2021-03-22T06:54:48Z



-def get_main_app(argv=[]):
+def get_main_app(argv=None):


Rationale: Function arguments should not be mutable.

Cerno-b · 2021-03-22T06:55:45Z


        left_index = (index + 1) % 4
        right_index = (index + 3) % 4
-        left_shift = None


Rationale: Variables are overwritten in all following paths, so they can be removed

Cerno-b · 2021-03-22T06:56:19Z

            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()))


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

Cerno-b · 2021-03-22T06:56:50Z


 class ComboBox(QWidget):
-    def __init__(self, parent=None, items=[]):
+    def __init__(self, parent=None, items=None):


Rationale: Function arguments should not be mutable

Cerno-b · 2021-03-22T06:57:11Z

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

-    def __init__(self, filename=None):


Rationale: Removed unused function argument

Cerno-b · 2021-03-22T07:00:03Z

        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):


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.

Cerno-b · 2021-03-22T07:01:47Z

        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}


Rationale: Unified the definition into a single dictionary creation call

Cerno-b · 2021-03-22T07:02:10Z

    def save(self, target_file=None):
        root = self.gen_xml()
        self.append_objects(root)
-        out_file = None


Rationale: Variable is overwritten in all paths, so it can be removed

Cerno-b · 2021-03-22T07:03:50Z

        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


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

Cerno-b · 2021-03-22T07:04:54Z

-                key = key_value[0].strip()
-                value = PROP_SEPERATOR.join(key_value[1:]).strip().strip('"')
-                self.id_to_message[key] = value
+                while not text.atEnd():


Rationale: This would have lead to a crash if f.open() failed.

Cerno-b · 2021-03-22T07:05:37Z

    return QRegExpValidator(QRegExp(r'^[^ \t].+'), None)


-class Struct(object):


Rationale: See https://github.com/tzutalin/labelImg/pull/723/files#r598455966

Cerno-b · 2021-03-22T07:06:15Z

    """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():


Rationale: Remove unused function

Cerno-b · 2021-03-22T07:06:36Z

-def util_qt_strlistclass():
-    return QStringList if have_qstring() else list

-def natural_sort(list, key=lambda s:s):


Rationale: Avoid using reserved names in variables

Cerno-b · 2021-03-22T07:06:46Z

    """
    Sort the list into natural alphanumeric order.
    """
-    def get_alphanum_key_func(key):


Rationale: Avoid using reserved names in variables

Cerno-b · 2021-03-22T07:07:14Z

-        return lambda s: [convert(c) for c in re.split('([0-9]+)', key(s))]
+    def get_alphanum_key_func(internal_key):
+
+        def convert(text):


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

Cerno-b added 15 commits March 8, 2021 00:56

rename local variables in main file

923e982

additional renaming of functions and variables

d5fcc23

Rename main file functions

85d3b3d

Rename functions and variables in canvas.py

9582b53

Rename functions and locals for remaining files

a8beb89

Rename non-Qt derived class' members

39b71f8

Rename members of Qt-derived classes

ba2792c

Fix paint label issue

4812be5

Fix more warnings

9b7c696

Further warning removal and cleanup

0bafad5

Rename filenames

6f9f639

Merge branch 'main' into pep-8

d042256

Merge branch 'pep-8' into pep-8_stage_2

e9ee3e0

Unify action var names and fix some camel case

83e5c5a

Fix indentation

4f6029e

Cerno-b commented Mar 22, 2021

View reviewed changes

Cerno-b mentioned this pull request Mar 22, 2021

CreateML annotation format does not work. #724

Closed

Cerno-b commented Mar 22, 2021

View reviewed changes

Cerno-b mentioned this pull request Mar 22, 2021

Fix more PEP 8 findings and warnings #725

Closed

Cerno-b changed the title ~~Fix warnings~~ Fix more PEP 8 findings and warnings Mar 22, 2021

Cerno-b mentioned this pull request Mar 22, 2021

Switch to pep-8 #717

Closed

		return QRegExpValidator(QRegExp(r'^[^ \t].+'), None)


		class Struct(object):

Conversation

Cerno-b commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cerno-b commented Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading

Cerno-b Mar 22, 2021 •

edited

Loading