PR: Cleaning up code #94

goanpeca · 2016-09-21T21:24:41Z

No description provided.

Nodd

I did a quick pass on the code. A few comments here and here but overall the code looks more clear and more pythonic after your changes.

Nodd · 2016-09-23T19:42:54Z

conda_manager/models/packages.py

@@ -275,17 +268,17 @@ def data(self, index, role=Qt.DisplayRole):
                    color = palette.color(QPalette.Mid)
                    color = P['foreground.not.installed']
                    return to_qvariant(color)
-            elif column in [C.COL_VERSION]:


Use == here since there is only one choice ?

Nodd · 2016-09-23T19:43:24Z

bootstrap.py

+#
+# Licensed under the terms of the MIT License
+# -----------------------------------------------------------------------------
+"""Bottstrap script."""


Typo in Bootstrap

Nodd · 2016-09-23T19:44:09Z

conda_manager/models/packages.py

-        elif role == Qt.SizeHintRole:
-            if column in [C.ACTION_COLUMNS] + [C.COL_PACKAGE_TYPE]:
+        elif (role == Qt.SizeHintRole and column in
+              C.ACTION_COLUMNS + [C.COL_PACKAGE_TYPE]):


The semantic is changed for the addition here, is it normal ?

Yeah it was a mistae :-|

Nodd · 2016-09-23T19:44:51Z

conda_manager/utils/__init__.py

@@ -129,7 +129,7 @@ def toint(x):
    # replace letters found by a negative number
    replace_dic = {}
    alpha = sorted(alpha, reverse=True)
-    if len(alpha):
+    if alpha:


The sort can be done inside the if block I guess ?

Nodd · 2016-09-23T19:47:06Z

conda_manager/widgets/main_window.py

@@ -116,23 +116,28 @@ def set_environments(self, prefix):
        """ """
        self.packages.set_environment(prefix=prefix)

-    def add_env(self):
+    @staticmethod
+    def add_env():


These shouldn't be removed since I guess that self will be used in the methods. Did you use an automated program to clean the code ?

Nodd · 2016-09-23T19:48:02Z

conda_manager/widgets/packages.py

@@ -163,7 +165,7 @@ def __init__(self,
        self._parent = parent
        self._current_action_name = ''
        self._hide_widgets = False
-        self._metadata = extra_metadata  # From repo.continuum
+        self._metadata = extra_metadata if extra_metadata else {}


Maybe extra_metadata or {} instead ? It's a common idiom in python.

Nodd · 2016-09-23T19:48:32Z

conda_manager/widgets/packages.py

-            self._metadata = metadata
+        else:
+            blacklist = []
+        self._metadata = metadata if metadata else {}


Same as before

Nodd · 2016-09-23T19:48:55Z

conda_manager/widgets/packages.py

        """ """
        # If environment exists already? GUI should take care of this
        # BUT the api call should simply set that env as the env
        dic = {}
+        packages = packages if packages else ['python']


Same again.

Nodd · 2016-09-23T19:50:44Z

conda_manager/widgets/search.py

            self.button_clear.setVisible(True)
+        else:
+            self.button_clear.setVisible(False)


What about:

visibility = self.text() # or bool(self.text()) to help the reader understand the intent self.button_clear.setVisible(visibility)

It's shorter and looks less naive.

Nodd · 2016-09-23T19:51:01Z

conda_manager/widgets/table.py

        """ """
        self.proxy_model = MultiColumnSortFilterProxy(self)
        self.source_model = CondaPackagesModel(self, packages, data)
        self.proxy_model.setSourceModel(self.source_model)
        self.setModel(self.proxy_model)
-        self.metadata_links = metadata_links
+        self.metadata_links = metadata_links if metadata_links else {}


goanpeca · 2016-09-23T20:21:34Z

Thanks for the comments @Nodd will fix them and merge.

goanpeca added 10 commits September 21, 2016 17:24

Remove mutable default values

2929e1c

Avoid None as redundant second argument to dict.get()

7a0053d

Replace list by generator

c62ebfe

Replace lambdas by nested functions

bd5bf43

Avoid using to check if a list is empty

6f2bc48

Avoid unnecessary name repetition in equality comparisions

acb4f8e

Add license code and use format instead of %

4d6f7d8

Use format instead of %

7699178

Use static methods when needed

6597a9a

Avoid consecutive ifs

b884363

Nodd reviewed Sep 23, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR: Cleaning up code #94

PR: Cleaning up code #94

goanpeca commented Sep 21, 2016

Nodd left a comment

Nodd Sep 23, 2016

goanpeca Sep 23, 2016

Nodd Sep 23, 2016

goanpeca Sep 23, 2016

Nodd Sep 23, 2016

goanpeca Sep 23, 2016

Nodd Sep 23, 2016

Nodd Sep 23, 2016

Nodd Sep 23, 2016

Nodd Sep 23, 2016

Nodd Sep 23, 2016

Nodd Sep 23, 2016 •

edited

Loading

Nodd Sep 23, 2016

goanpeca commented Sep 23, 2016

PR: Cleaning up code #94

Are you sure you want to change the base?

PR: Cleaning up code #94

Conversation

goanpeca commented Sep 21, 2016

Nodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nodd Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Sep 23, 2016

Nodd Sep 23, 2016 •

edited

Loading