[MRG + 1] Make pep8 #125

Oshawk · 2018-10-04T19:40:36Z

Have tried to adhere to pep8 but kept things that looked like a stylistic decision or or things like long URLs that can't really be changed.

A couple of things I have not done:

Made function variables lowercase (would require major refactoring).
Removed unused arguments and variables (didn't want to break anything).
Changed the shift_text argument of __init__ in Lattice to a tuple as lists are mutable and thus bad practice (again, not sure what it might break).

codecov-io · 2018-10-04T19:45:00Z

Codecov Report

Merging #125 into master will decrease coverage by 0.06%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
- Coverage   84.28%   84.22%   -0.07%     
==========================================
  Files          12       12              
  Lines        1203     1198       -5     
  Branches      288      288              
==========================================
- Hits         1014     1009       -5     
  Misses        145      145              
  Partials       44       44

Impacted Files	Coverage Δ
camelot/core.py	`86.36% <ø> (-0.06%)`	⬇️
camelot/io.py	`100% <ø> (ø)`	⬆️
camelot/parsers/__init__.py	`100% <ø> (ø)`	⬆️
camelot/utils.py	`81.25% <ø> (-0.14%)`	⬇️
camelot/handlers.py	`83.33% <ø> (ø)`	⬆️
camelot/parsers/base.py	`100% <ø> (ø)`	⬆️
camelot/__version__.py	`100% <ø> (ø)`	⬆️
camelot/plotting.py	`16% <ø> (ø)`	⬆️
camelot/image_processing.py	`85.29% <100%> (-0.43%)`	⬇️
camelot/parsers/stream.py	`90.8% <100%> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e8079d...6c4946e. Read the comment docs.

vinayak-mehta · 2018-10-04T20:19:42Z

@Oshawk The PR looks great! I had a couple of changes to request (will tag the smaller ones in the review).

Please use one of the last two styles (and not the first one) suggested by pep8 — indentation wherever applicable. (tag - indent)
I've grouped the imports as suggested by pep8 — imports, in the following order 1. standard library, 2. third party and 3. local relative imports. Saw a case where this wasn't being followed, can you change that? (tag - import)

Can you give me examples of long URLs and uppercase function variables? Also, please take a look at the contribution guidelines, specifically adding a prefix to the PR title and Capitalizing the commit message. Thanks!

vinayak-mehta · 2018-10-04T20:21:44Z

camelot/__init__.py

@@ -1,4 +1,6 @@
 # -*- coding: utf-8 -*-
+from .__version__ import __version__


vinayak-mehta · 2018-10-04T20:21:59Z

camelot/cli.py

@@ -1,18 +1,17 @@
 # -*- coding: utf-8 -*-
+from . import __version__


vinayak-mehta · 2018-10-04T20:22:13Z

camelot/image_processing.py

@@ -43,10 +43,10 @@ def adaptive_threshold(imagename, process_background=False, blocksize=15, c=-2):

    if process_background:
        threshold = cv2.adaptiveThreshold(gray, 255, cv2.ADAPTIVE_THRESH_GAUSSIAN_C,
-            cv2.THRESH_BINARY, blocksize, c)
+                                          cv2.THRESH_BINARY, blocksize, c)


vinayak-mehta · 2018-10-04T20:23:18Z

camelot/image_processing.py

@@ -180,7 +180,7 @@ def find_table_joints(contours, vertical, horizontal):
    tables = {}
    for c in contours:
        x, y, w, h = c
-        roi = joints[y : y + h, x : x + w]
+        roi = joints[y: y + h, x: x + w]


pep8 suggests the earlier version.

vinayak-mehta · 2018-10-04T20:23:31Z

camelot/image_processing.py

@@ -263,13 +263,13 @@ def find_cuts(threshold, char_size_scaling=200):

    try:
        __, contours, __ = cv2.findContours(threshold, cv2.RETR_EXTERNAL,
-            cv2.CHAIN_APPROX_SIMPLE)
+                                            cv2.CHAIN_APPROX_SIMPLE)


vinayak-mehta · 2018-10-04T20:23:52Z

camelot/parsers/lattice.py

@@ -183,7 +183,7 @@ def _generate_image(self):
        else:
            gs_call.insert(0, "gsc")
        subprocess.call(gs_call, stdout=open(os.devnull, 'w'),
-            stderr=subprocess.STDOUT)
+                        stderr=subprocess.STDOUT)


vinayak-mehta · 2018-10-04T20:24:03Z

camelot/parsers/lattice.py

@@ -320,9 +320,9 @@ def extract_tables(self, filename):
        _tables = []
        # sort tables based on y-coord
        for table_idx, tk in enumerate(sorted(self.table_bbox.keys(),
-                key=lambda x: x[1], reverse=True)):
+                                              key=lambda x: x[1], reverse=True)):


vinayak-mehta · 2018-10-04T20:24:15Z

camelot/parsers/stream.py

@@ -358,9 +358,9 @@ def extract_tables(self, filename):
        _tables = []
        # sort tables based on y-coord
        for table_idx, tk in enumerate(sorted(self.table_bbox.keys(),
-                key=lambda x: x[1], reverse=True)):
+                                              key=lambda x: x[1], reverse=True)):


vinayak-mehta · 2018-10-04T20:24:53Z

tests/test_common.py

@@ -49,7 +49,7 @@ def test_lattice():
    df = pd.DataFrame(data_lattice)

    filename = os.path.join(testdir,
-        "tabula/icdar2013-dataset/competition-dataset-us/us-030.pdf")
+                            "tabula/icdar2013-dataset/competition-dataset-us/us-030.pdf")


Oshawk · 2018-10-04T20:47:22Z

Will do this tomorrow!

Oshawk · 2018-10-05T09:07:31Z

There are lots of capital letters in core.py. E.g. J in line 189. The long URLs are mainly in docstrings. E.g. image_processing.py line 24.

Also really sorry for not capitulating the commits. Did not notice that part until they were made.

vinayak-mehta · 2018-10-05T09:31:53Z

There are lots of capital letters in core.py. E.g. J in line 189. The long URLs are mainly in docstrings. E.g. image_processing.py line 24.

Yeah, kept them uppercase to signify that they're counterparts of the same lowercase letters. (J <-> j)

Also really sorry for not capitulating the commits. Did not notice that part until they were made.

No worries, you can still reword the commit if you want. Check this GitHub article for how to do that.

Thanks for the changes, I'll merge this after I merge the other tests PR. I suspect that some things might break, but I'll look into that!

Add new line at end of file, fix bare except, remove unused import.

Add some newlines at and of files and a visual indent.

Fix block comments and add new lines at end of files.

Fixed unused import, a few weirdly ordered imports, a docstring typo and many new lines at the end of lines.

Fix import order and remove a couple more unused imports.

Fix indentation (no opening delimiter alignment).

vinayak-mehta · 2018-10-05T11:25:54Z

@Oshawk Thanks for the contribution!

Oshawk · 2018-10-05T12:00:52Z

@vinayak-mehta You are welcome! Glad to help.

Oshawk mentioned this pull request Oct 4, 2018

Make sure code follows pep8 #123

Closed

vinayak-mehta self-requested a review October 4, 2018 20:20

vinayak-mehta suggested changes Oct 4, 2018

View reviewed changes

Oshawk changed the title ~~Pep8~~ [MRG] Pep8 Oct 5, 2018

Oshawk added 6 commits October 5, 2018 16:19

Make setup.py pep8

7505139

Add new line at end of file, fix bare except, remove unused import.

Make tests/*.py pep8

c04fb85

Add some newlines at and of files and a visual indent.

Make docs/*.py pep8

05d6890

Fix block comments and add new lines at end of files.

Make camelot/*.py pep8

6b7b7da

Fixed unused import, a few weirdly ordered imports, a docstring typo and many new lines at the end of lines.

Fix imports

2e4316d

Fix import order and remove a couple more unused imports.

Fix indents

33544dc

Fix indentation (no opening delimiter alignment).

vinayak-mehta approved these changes Oct 5, 2018

View reviewed changes

vinayak-mehta changed the title ~~[MRG] Pep8~~ [MRG] Make pep8 Oct 5, 2018

Add newlines

6c4946e

vinayak-mehta changed the title ~~[MRG] Make pep8~~ [MRG + 1] Make pep8 Oct 5, 2018

vinayak-mehta merged commit 90aaba6 into atlanhq:master Oct 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG + 1] Make pep8 #125

[MRG + 1] Make pep8 #125

Oshawk commented Oct 4, 2018

codecov-io commented Oct 4, 2018 •

edited

Loading

vinayak-mehta commented Oct 4, 2018 •

edited

Loading

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

vinayak-mehta Oct 4, 2018

Oshawk commented Oct 4, 2018

Oshawk commented Oct 5, 2018 •

edited

Loading

vinayak-mehta commented Oct 5, 2018

vinayak-mehta commented Oct 5, 2018

Oshawk commented Oct 5, 2018

		@@ -1,4 +1,6 @@
		# -- coding: utf-8 --
		from .__version__ import __version__

		@@ -1,18 +1,17 @@
		# -- coding: utf-8 --
		from . import __version__

[MRG + 1] Make pep8 #125

[MRG + 1] Make pep8 #125

Conversation

Oshawk commented Oct 4, 2018

codecov-io commented Oct 4, 2018 • edited Loading

Codecov Report

vinayak-mehta commented Oct 4, 2018 • edited Loading

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

Oshawk commented Oct 4, 2018

Oshawk commented Oct 5, 2018 • edited Loading

vinayak-mehta commented Oct 5, 2018

vinayak-mehta commented Oct 5, 2018

Oshawk commented Oct 5, 2018

codecov-io commented Oct 4, 2018 •

edited

Loading

vinayak-mehta commented Oct 4, 2018 •

edited

Loading

Oshawk commented Oct 5, 2018 •

edited

Loading