PR: Static analysis pass. by KelSolaar · Pull Request #82 · gtaylor/python-colormath

KelSolaar · 2018-01-29T05:35:08Z

I performed an first static analysis pass over the codebase with PyCharm and Codacy.

rec

Just a few comments from a bystander...

rec · 2018-01-29T08:54:35Z


        lchab = LCHabColor(40.0, 104.0, 40.0)
-        rgb = convert_color(lchab, AdobeRGBColor)
+        _rgb = convert_color(lchab, AdobeRGBColor)


This change seems pointless, as rgb is a local variable.

Agreed. We're also a bit inconsistent with `test_conversion_to_rgb_zero_div and others now, too.

rec · 2018-01-29T08:56:52Z

-    hsv_h = var_H
-    hsv_s = var_S
-    hsv_v = var_V
+    # TODO: Investigate intent of following code block.


You're simply making work for the next person. Those variables are not used - just delete them.

You're simply making work for the next person.

The next person would actually be me and the note is actually for me. To give you some context I have been discussing with @gtaylor during the last few weeks to take over the maintenance of python-colormath.

We probably could go on ahead and remove these. If we're worried about needing to back them out in the future, these can be done in atomic, easily-reverted commits.

rec · 2018-01-29T08:59:48Z

-        rgb_type = target_rgb
-    else:
-        rgb_type = cobj.rgb_type
+    # if target_rgb is not None:


You are changing the meaning of the code here, by commenting out this block. Are you 100% sure that no clients use this functionality?

(Also, if you keep the code, it could be more easily written as rgb_type = target_rgb or cobj.rgb_type.)

rgb_type is not used in the remaining definition code and I have just noticed that I missed a similar construct here: https://github.com/colour-science/python-colormath/blob/6d4ac23c7913974f6d5a2072b17ea94fb62ced24/colormath/color_conversions.py#L769

I introspected the codebase with a debugger to understand what was the intent of the code block without much clue, commented the block and the unit tests are running successfully.

If there is a hidden magical behaviour, I would be happy to know about it.

rec · 2018-01-29T09:08:51Z

@@ -1 +1,3 @@
+#!/usr/bin/env python


I don't understand why you are adding this first line to all the files. You aren't ever going to call __init__.py directly as a script, and you couldn't do it anyway because it isn't marked as +x and because you don't have a if __name__ == '__main': clause.

Generally, you add #!/usr/bin/env python to the top of Python files exactly if they are going to be called as standalone scripts. Typically this is only for a couple of files in a project.

Hi @rec!

I brought this line along with the encoding one. I actually needed and still need to launch quickly any of the modules, i.e. good when introspecting a codebase. You don't need a if __name__ == '__main': clause to run content of any Python module and on Windows you don't need to make the script executable at all, see PEP 397.

The fact the line is here does not really hurt and makes some things easier for me.

Definitely understand how it may help your particular workflow, but this is a bit of an anti-pattern. We should probably be optimizing worfkflows around the test suite.

gtaylor

Many apologies for the delay, and many thanks for the PR! Just a few assorted comments.

gtaylor · 2018-02-12T04:26:09Z

-    hsv_h = var_H
-    hsv_s = var_S
-    hsv_v = var_V
+    # TODO: Investigate intent of following code block.


We probably could go on ahead and remove these. If we're worried about needing to back them out in the future, these can be done in atomic, easily-reverted commits.

gtaylor · 2018-02-12T04:26:48Z

    else:
        raise ValueError("Unable to convert HSL->RGB due to value error.")

+    # TODO: Investigate intent of following code block.


I'd suggest an issue tracker with a link to the line, pinned to a certain git sha (to prevent code shifting from breaking your link).

gtaylor · 2018-02-12T04:28:08Z


        lchab = LCHabColor(40.0, 104.0, 40.0)
-        rgb = convert_color(lchab, AdobeRGBColor)
+        _rgb = convert_color(lchab, AdobeRGBColor)


Agreed. We're also a bit inconsistent with `test_conversion_to_rgb_zero_div and others now, too.

gtaylor · 2018-02-12T04:30:18Z

    # In case the white-points are still input as strings
    # Get white-points for illuminant
-    if type(wp_src) == str:
+    if isinstance(wp_src, str):


Worth considering basestring instead?

basestring does not exist in Python 3.x so we would need to either account for it or use six which is cleaner and more portable.

gtaylor · 2018-02-12T04:32:10Z

@@ -1 +1,3 @@
+#!/usr/bin/env python


Definitely understand how it may help your particular workflow, but this is a bit of an anti-pattern. We should probably be optimizing worfkflows around the test suite.

KelSolaar · 2018-02-12T07:29:41Z

I have implemented the code review changes except for the basestring stuff as per my above comment.

gtaylor

Looks good, thanks! Just make sure you squash/clean up the commit history before merge!

Squashed commits: [22a8ba5] Remove unused variables in "colormath.color_conversions.RGB_to_HSV" definition. [ecf5216] Handle and add "TODO" note to investigate remaining unused variable. [0978310] Remove interpreter shebangs. [df93e8d] Specify modules encoding. [b0e2ef4] Fix "colormath.density.ansi_density" definition docstring. [d2fc110] Fix incorrect string formatting in "colormath.color_appearance_models.CIECAM02.__init__" method. [85a73f5] Improve type checking in various "colormath.chromatic_adaptation" module definitions. [0077e3e] Remove unused imports. [6d4ac23] Handle and add "TODO" notes to investigate various unused variables. [200caba] Fix trailing whitespaces.

KelSolaar · 2018-02-14T06:28:24Z

@gtaylor: /done!

gtaylor

Feel free to merge at your leisure

rec reviewed Jan 29, 2018

View reviewed changes

gtaylor suggested changes Feb 12, 2018

View reviewed changes

KelSolaar force-pushed the feature/static_analysis branch from 7baf4d7 to 22a8ba5 Compare February 12, 2018 07:27

gtaylor approved these changes Feb 13, 2018

View reviewed changes

KelSolaar force-pushed the feature/static_analysis branch from 22a8ba5 to 189ec1a Compare February 14, 2018 06:28

gtaylor approved these changes Feb 18, 2018

View reviewed changes

KelSolaar merged commit 0f5e043 into gtaylor:master Feb 21, 2018

Conversation

KelSolaar commented Jan 29, 2018

Uh oh!

rec left a comment

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!

KelSolaar Jan 29, 2018 • 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!

KelSolaar Jan 29, 2018 • 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!

gtaylor left a comment

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!

KelSolaar commented Feb 12, 2018

Uh oh!

gtaylor left a comment

Choose a reason for hiding this comment

Uh oh!

KelSolaar commented Feb 14, 2018

Uh oh!

gtaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KelSolaar Jan 29, 2018 •

edited

Loading

KelSolaar Jan 29, 2018 •

edited

Loading