PEP8 formatting python code #124

isaac-philip · 2018-11-07T17:11:14Z

Made PEP8 changes using flake8.
Placed flake8 config in setup.cfg

WIP for #115

Next step is for including flake8 linting and stopping CI using travis etc as per maintainers direction.

isaac-philip · 2018-11-07T17:22:37Z

Please verify, this has the code related to the single quotes/double quotes which was earlier formatted using black.
All PEP8 modified code present.
Unfortunately still failing for some reason at probably same location as previous PR.
Please let me know what needs to change.
Thanks.

.gitignore

ehashman · 2018-11-08T14:14:31Z

auditwheel/lddtree.py

@@ -37,7 +36,7 @@ def normpath(path: str) -> str:
    return os.path.normpath(path).replace('//', '/')


-def readlink(path: str, root: str, prefixed: bool=False) -> str:
+def readlink(path: str, root: str, prefixed: bool = False) -> str:


I was trying to figure out where this change came from and realized this error is from a relatively new version of PEP8: python/peps@acda0d1

Guess we must comply 🤷‍♀️

ehashman

We will need to get the tests passing before this is mergeable. I'm not sure what change is causing the breakage. Are you able to run them locally?

auditwheel/lddtree.py

ehashman · 2018-11-08T14:16:17Z

auditwheel/main.py

@@ -47,7 +47,7 @@ def main():

    try:
        rval = args.func(args, p)
-    except:
+    except Exception:
        # TODO(rmcgibbo): nice message
        raise


This except section doesn't do anything, so we can remove the whole try: ... except: block in favour of just rval = args.func(args, p).

sure, the error was no Exception statement so I placed that.
Now will remove the entire try catch and replace with the function rval part only.

ehashman · 2018-11-08T15:18:37Z

auditwheel/wheel_abi.py

+    external_refs_by_fn = wheel_elfdata[1]
+    versioned_symbols = wheel_elfdata[2]
+    has_ucs2 = wheel_elfdata[3]
+    uses_PyFPE_jbuf = wheel_elfdata[4]


IMO we should really just change this function to return a dict instead of a tuple, describing what the return arguments are. Anything more than 2 or 3 posargs is difficult to make sense of and these new assignments sort of lay that bare.

ehashman · 2018-11-08T15:22:21Z

tests/test_manylinux.py

+    docker_exec(manylinux_id, ['bash', '-c',
+                               'cd /auditwheel_src/tests/testpackage',
+                               'python setup.py bdist_wheel -d /io']
+                )


This change is not semantically equivalent and should be reverted. It's not showing up as test breakage because something else appears to be causing the tests to fail.

The reason I removed the bash && operator and moved the python command to a separate argument is to avoid the 90 character spill. To me this looks good to achieve that. In the future pep8 tests will fail on this for max-line.

Let me know for your suggestions.

used flake8 for PEP8 linting 1. Modified multiple python files 2. Added flake8 configuration under setup.cfg WIP for pypa#115 Comments in PR, 1. Removed .vscode from gitignore file 2. Comments on PR resolved

isaac-philip · 2018-11-09T06:34:18Z

Made changes as per PR.
I haven't yet executed locally considering that this is just syntactical changes and basic imports etc modified.
If there will be a benefit by doing so I will, let me know.

I don't see how the builds are failing with the changes though, give some light on that, thanks for your co-operation.

ehashman · 2018-11-13T03:04:48Z

If it doesn't pass locally, it certainly won't pass in CI 😄 I recommend working to fix the test failures locally before submitting to Travis as you will be able to iterate faster. I can't merge this PR as is because it causes auditwheel to stop working in its current state.

I've merged some PRs since this was first submitted so the merge conflicts will also need to be fixed.

isaac-philip · 2018-11-13T18:07:01Z

Yes @ehashman I will look to have it test locally first. Apologies for that, I cant understand what is the reason for mere syntactical changes as per PEP8 causing this but will check locally first then place it here, thanks.

ehashman · 2018-11-14T01:44:03Z

Yeah it doesn't make a ton of sense to me either; you might want to make sure that your code is rebased onto master. git bisect can be a useful tool for locating where the problem arose.

isaac-philip · 2018-11-24T18:28:06Z

Hello @ehashman ,
I attempted to solve it but facing impediment at same step.
Would like to know which step am I missing.

within auditwheel directory,
1$ pip install -r requirements.txt
2$ pip install .
3$ ./tests/travis.sh
Still breaks at the certain line for importing load_policies.

Please let me know what I am doing incorrect.
Thanks.

lkollar

The build is probably failing because of a recursive import here: https://github.com/pypa/auditwheel/pull/124/files#diff-4629f407e7791be41dc48487a65a2653R8

I've actually submitted #158 to add flake8 before noticing this PR.

lkollar · 2019-05-07T18:49:03Z

#158 has been merged which implements most of this. @isaac-philip if you think anything is missing from #158 which was part of your PR please open a new issue/PR.

isaac-philip mentioned this pull request Nov 7, 2018

Add flake8 to CI #115

Closed

atombrella reviewed Nov 7, 2018

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

ehashman reviewed Nov 8, 2018

View reviewed changes

ehashman requested changes Nov 8, 2018

View reviewed changes

ehashman mentioned this pull request Nov 8, 2018

WIP for #115 : PEP8 refactor, CI to be added #116

Closed

isaac-philip force-pushed the temp-working branch from 1ac6179 to 0cb9959 Compare November 9, 2018 06:21

Refactor code as per PEP8

9ec3f6b

used flake8 for PEP8 linting 1. Modified multiple python files 2. Added flake8 configuration under setup.cfg WIP for pypa#115 Comments in PR, 1. Removed .vscode from gitignore file 2. Comments on PR resolved

isaac-philip force-pushed the temp-working branch from 0cb9959 to 9ec3f6b Compare November 9, 2018 06:29

lkollar reviewed May 4, 2019

View reviewed changes

lkollar closed this May 7, 2019

isaac-philip deleted the temp-working branch September 5, 2019 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PEP8 formatting python code #124

PEP8 formatting python code #124

isaac-philip commented Nov 7, 2018

isaac-philip commented Nov 7, 2018

ehashman Nov 8, 2018

ehashman left a comment

ehashman Nov 8, 2018

isaac-philip Nov 9, 2018

ehashman Nov 8, 2018

isaac-philip Nov 9, 2018

ehashman Nov 8, 2018

isaac-philip Nov 9, 2018

isaac-philip commented Nov 9, 2018

ehashman commented Nov 13, 2018

isaac-philip commented Nov 13, 2018

ehashman commented Nov 14, 2018

isaac-philip commented Nov 24, 2018

lkollar left a comment

lkollar commented May 7, 2019

PEP8 formatting python code #124

PEP8 formatting python code #124

Conversation

isaac-philip commented Nov 7, 2018

isaac-philip commented Nov 7, 2018

ehashman Nov 8, 2018

Choose a reason for hiding this comment

ehashman left a comment

Choose a reason for hiding this comment

ehashman Nov 8, 2018

Choose a reason for hiding this comment

isaac-philip Nov 9, 2018

Choose a reason for hiding this comment

ehashman Nov 8, 2018

Choose a reason for hiding this comment

isaac-philip Nov 9, 2018

Choose a reason for hiding this comment

ehashman Nov 8, 2018

Choose a reason for hiding this comment

isaac-philip Nov 9, 2018

Choose a reason for hiding this comment

isaac-philip commented Nov 9, 2018

ehashman commented Nov 13, 2018

isaac-philip commented Nov 13, 2018

ehashman commented Nov 14, 2018

isaac-philip commented Nov 24, 2018

lkollar left a comment

Choose a reason for hiding this comment

lkollar commented May 7, 2019