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

Py3 prints #73

Merged
merged 3 commits into from Sep 24, 2016
Merged

Py3 prints #73

merged 3 commits into from Sep 24, 2016

Conversation

zuphilip
Copy link
Collaborator

@zuphilip zuphilip commented Sep 23, 2016

The tsht-tests works for me locally with Python 2 as well as Python 3 and also Travis agreed. However, let us check this carefully and test some more examples.

@stweil
Copy link
Collaborator

stweil commented Sep 23, 2016

2to3 does what the name says, namely convert code which was written for Python 2 to code which works with Python 3. It does not promise that it will still work with Python 2 after the conversion. Even if it seems to work at a first glance, here is an example of the problems with Python 2:

$ ./hocr-check -h
('usage:', './hocr-check', '[-o] file.html')

In Python 2 print is not a function, so the output includes everything which follows the print keyword which is not what we expect here.

I only see two possible solutions to support both versions of Python:

  • Conditional code, that means one print statement for each version.
  • Additional wrapper library. This is the commonly used solution.

@zuphilip
Copy link
Collaborator Author

Ah, okay, thank you for the explanation. One can include in every file with a print statement at the very beginning the statement:

from __future__ import print_function

I guess this is the wrapper library you mean?

@stweil
Copy link
Collaborator

stweil commented Sep 23, 2016

Yes.

@@ -35,4 +36,4 @@ for fname in sys.argv[2:]:
page = doc.importNode(page,1)
container.append(page)

print(etree.tostring(doc, pretty_print=True))
print((etree.tostring(doc, pretty_print=True)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess this is not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it should be removed.

@zuphilip
Copy link
Collaborator Author

Okay, I added the wrapper library to the files. Thus, we can test and discuss this further.

@@ -61,4 +62,4 @@ for node in dc_nodes:
hnode.attrib['content'] = value
hocr_meta.append(hnode)

print(etree.tostring(hocr_doc, pretty_print=True))
print((etree.tostring(hocr_doc, pretty_print=True)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is also not needed.

@@ -136,6 +137,6 @@ CMGjwvxTsr74/f/F95m3TH9x8o0/TU//N+7/D/ScVcA=

if __name__ == "__main__":
if len(sys.argv) == 1:
print("Usage: %s <imgdir>\n" % os.path.basename(sys.argv[0]))
print(("Usage: %s <imgdir>\n" % os.path.basename(sys.argv[0])))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed, too.

@zuphilip
Copy link
Collaborator Author

I deleted the three unnecessary brackets. Please have a look at the new version.

@stweil
Copy link
Collaborator

stweil commented Sep 24, 2016

@zuphilip, I changed the order of the commits and squashed two of them. This avoids unnecessary commits and asserts that the single commits don't break anything. I think that the PR is now ready to get merged.

@zuphilip
Copy link
Collaborator Author

Fine for me! Thanks for squashing the commits and reviewing.

@stweil stweil merged commit 6632b1b into ocropus:master Sep 24, 2016
@stweil stweil deleted the py3-prints branch September 24, 2016 10:13
@stweil
Copy link
Collaborator

stweil commented Sep 24, 2016

Thank you for this PR. Merged now.

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.

None yet

2 participants