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.

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

[MRG + 1] Make pep8 #125

Merged
merged 7 commits into from
Oct 5, 2018
Merged

[MRG + 1] Make pep8 #125

merged 7 commits into from
Oct 5, 2018

Conversation

Oshawk
Copy link
Contributor

@Oshawk Oshawk commented Oct 4, 2018

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
Copy link

codecov-io commented Oct 4, 2018

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor

vinayak-mehta commented Oct 4, 2018

@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 vinayak-mehta self-requested a review October 4, 2018 20:20
@@ -1,4 +1,6 @@
# -*- coding: utf-8 -*-
from .__version__ import __version__
Copy link
Contributor

Choose a reason for hiding this comment

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

[import]

camelot/cli.py Outdated
@@ -1,18 +1,17 @@
# -*- coding: utf-8 -*-
from . import __version__
Copy link
Contributor

Choose a reason for hiding this comment

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

[import]

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

[indent]

@@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

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

pep8 suggests the earlier version.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

[indent]

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

[indent]

@@ -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)):
Copy link
Contributor

Choose a reason for hiding this comment

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

[indent]

@@ -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)):
Copy link
Contributor

Choose a reason for hiding this comment

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

[indent]

@@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

[indent]

@Oshawk
Copy link
Contributor Author

Oshawk commented Oct 4, 2018

Will do this tomorrow!

@Oshawk Oshawk changed the title Pep8 [MRG] Pep8 Oct 5, 2018
@Oshawk
Copy link
Contributor Author

Oshawk commented Oct 5, 2018

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
Copy link
Contributor

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 vinayak-mehta changed the title [MRG] Pep8 [MRG] Make pep8 Oct 5, 2018
@vinayak-mehta vinayak-mehta changed the title [MRG] Make pep8 [MRG + 1] Make pep8 Oct 5, 2018
@vinayak-mehta vinayak-mehta merged commit 90aaba6 into atlanhq:master Oct 5, 2018
@vinayak-mehta
Copy link
Contributor

@Oshawk Thanks for the contribution!

@Oshawk
Copy link
Contributor Author

Oshawk commented Oct 5, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants