Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flake8 code style fixes #241

Closed
wants to merge 1 commit into from
Closed

Flake8 code style fixes #241

wants to merge 1 commit into from

Conversation

roaffix
Copy link
Contributor

@roaffix roaffix commented Nov 13, 2020

  • Fix flake8 errors
  • Fix some FIXME tags
  • Move to_str method to library from utils to avoid import errors

@roaffix roaffix changed the base branch from master to dev November 13, 2020 04:54
@roaffix roaffix changed the title Flake8 code stype fixes Flake8 code style fixes Nov 13, 2020
@roaffix roaffix force-pushed the fix/flake8-refactoring branch from 0d9164c to 04d50ce Compare November 13, 2020 04:57
9prady9
9prady9 reviewed Nov 13, 2020
View reviewed changes
Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

Most of utility abstractions look good although it seems majority of if checks are now contradicting PEP8 style in the following two patterns:

  1. if <var> is None: or if <var> is not None should be preferred over if <var>: per PEP8 guidline - https://www.python.org/dev/peps/pep-0008/#programming-recommendations
  2. if <var> == 0 or <some number> - not sure what's PEP8 stance on this type of comparisons, but numerical comparisons are better written this way to ensure better code readability and understanding instead of if <var>: which seems like checking for None and may lead to confusion.

@roaffix roaffix force-pushed the fix/flake8-refactoring branch 2 times, most recently from 16f3c01 to 9b328f1 Compare November 13, 2020 17:18
@roaffix
Copy link
Contributor Author

roaffix commented Nov 13, 2020

Most of utility abstractions look good although it seems majority of if checks are now contradicting PEP8 style in the following two patterns:

  1. if <var> is None: or if <var> is not None should be preferred over if <var>: per PEP8 guidline - https://www.python.org/dev/peps/pep-0008/#programming-recommendations
  2. if <var> == 0 or <some number> - not sure what's PEP8 stance on this type of comparisons, but numerical comparisons are better written this way to ensure better code readability and understanding instead of if <var>: which seems like checking for None and may lead to confusion.

You are right. Some variables are considered to be None by default, so it should be checked. Nice catch! I'll fix that.
In other cases, some of your checks, e.g., AF_PATH in library.py check if AF_PATH is None, but it should fail if AF_PATH == "". Simple conditions change to if AF_PATH can fix such problems when the default value or variable type is not known.

@roaffix roaffix force-pushed the fix/flake8-refactoring branch from 9b328f1 to ad77595 Compare November 14, 2020 00:12
syurkevi
syurkevi reviewed Nov 14, 2020
View reviewed changes
arrayfire/array.py
location = Source.device
else:
location = Source.host
location = Source.device if is_device else Source.host
Copy link
Contributor

Choose a reason for hiding this comment

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

revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, it's better now :)

arrayfire/arith.py
high_arr = constant_array(high, vdims[0], vdims[1], vdims[2], vdims[3], vty)
else:
high_arr = high.arr
low_arr = low.arr if is_low_array else constant_array(low, vdims[0], vdims[1], vdims[2], vdims[3], vty)
Copy link
Contributor

Choose a reason for hiding this comment

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

revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still, I don't see a point to revert these changes. These variables are just temp variables and they are not changed later anyhow. But we drop extra 7 lines of code.

@roaffix roaffix force-pushed the fix/flake8-refactoring branch from ad77595 to 6012303 Compare November 14, 2020 01:17
@roaffix roaffix closed this Nov 14, 2020
@roaffix roaffix force-pushed the fix/flake8-refactoring branch from 6012303 to 28a0f61 Compare November 14, 2020 03:27
@roaffix roaffix reopened this Nov 14, 2020
@roaffix roaffix force-pushed the fix/flake8-refactoring branch 4 times, most recently from c20136f to 911be16 Compare November 14, 2020 16:24
@roaffix roaffix force-pushed the fix/flake8-refactoring branch from 911be16 to b7cffa1 Compare November 14, 2020 16:25
@roaffix
Copy link
Contributor Author

roaffix commented Nov 14, 2020

I'm going to postpone this PR till #244 is merged. I will add the arg --flake8 to the pytest for automatic code style checks here right after.

@roaffix roaffix closed this Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@9prady9 9prady9 9prady9 left review comments

@syurkevi syurkevi syurkevi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@roaffix @9prady9 @syurkevi