-
Notifications
You must be signed in to change notification settings - Fork 168
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
partially making in py3 ready #232
Conversation
there are still lots of improvements to make: with open(path, "wr") as f: f.read / f.write lots of open files warnings... #231
|
||
if sys.version_info > (3,): | ||
long = int | ||
longType = long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need both longType
(lowercase initial) and LongType
(uppercase), both alias of int
on py3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh fixed 14446c2
raise FDKEnvError | ||
|
||
def runShellCmd(cmd): | ||
try: | ||
p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT).stdout | ||
log = p.read() | ||
return log | ||
return str(log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's wrong. log
returned by subprocess is a bytes string. On py3, if you do str(b"a")
without an encoding
argument, you get its repr "b'a'"
. You want to call decode
method of bytes with the encoding that the subprocess command is using to print to stdout (probably ascii or utf-8, but you have to know).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, this is really wrong... but it works. To my defence: the whole file is a mess and needs a decent fix up...
will fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed ed87e80
@@ -1388,7 +1394,7 @@ def getROS(fontPath): | |||
""" | |||
R=O=S=None | |||
fp = open(fontPath, "rb") | |||
data = fp.read(5000) | |||
data = str(fp.read(5000)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, wrong for the same reason I explained above. you're calling str
on a bytes string, but it looks like you want to decode it.
Just open the file with io.open
as text and pass the correct encoding. You'll read properly decoded (unicode) strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
@@ -13,7 +15,7 @@ | |||
Auto-hinting program for PostScript and OpenType/CFF fonts. | |||
""" | |||
|
|||
from ufoTools import kProcessedGlyphsLayerName, kProcessedGlyphsLayer | |||
from .ufoTools import kProcessedGlyphsLayerName, kProcessedGlyphsLayer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
careful with these relative imports. if the python module is meant to be run as a script, then you can't use relative imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if one attempts to run a python script containing a relative import, one gets
ValueError: Attempted relative import in non-package
(py2) or ModuleNotFoundError:... '__main__' is not a package
(py3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will revert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that the issue with scripts and relative imports only arises if one attempts to call the module as a script like in python afdko/Tools/SharedData/FDKScripts/autohint.py
.
If the module has a setuptools console_scripts
entry point defined in setup.py, then it will be imported as a module and it's main() will be called from the wrapper script. So in this case, the name of the module is not __main__
but the full qualified module name, and any relative imports should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted f3246a6
there are still lots of improvements to make:
with open(path, "wr") as f:
f.read / f.write
lots of open files warnings...
#231