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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python3.8 Error when reading a file proposed fix #3

Closed
FredHappyface opened this issue Apr 21, 2020 · 4 comments 路 Fixed by #4
Closed

Python3.8 Error when reading a file proposed fix #3

FredHappyface opened this issue Apr 21, 2020 · 4 comments 路 Fixed by #4

Comments

@FredHappyface
Copy link
Contributor

Thanks for an absolutely brilliant project. And thanks for dumping it on
pypi.org so I can have a play around with it!

Unfortunately, ast has changed some of its function signatures 馃槧 which has
lead to an incompatibility with python3.8

I thought it may be worth contributing the patch, since I am using this in one
of my projects (note, I have dropped support for python2 as my project isn't
compatible with that)

The question is, can I make a pull request with this? Of course, I will keep the
python2 compatible element as I'm aware that it's still quite widely used.

from pypdn.namedlist import _make_fn
_make_fn = makeFn_PDN_fix

def makeFn_PDN_fix(name, chain_fn, args, defaults):
	"""Patch the function pypdn.namedlist._make_fn with a python 3.8 compatible
	version
	This snippet is MIT License Copyright (c) 2018 Addison Elliott
	"""
	import ast
	import sys
	args_with_self = ['_self'] + list(args)
	arguments = [ast.Name(id=arg, ctx=ast.Load()) for arg in args_with_self]
	defs = [ast.Name(id='_def{0}'.format(idx), ctx=ast.Load()) for idx, _ in enumerate(defaults)]
	# Python 3.8 version
	if sys.version_info[0] >= 3 and sys.version_info >= 8:
		parameters = ast.arguments(args=[ast.arg(arg=arg) for arg in args_with_self],
		posonlyargs=[],	kwonlyargs=[], defaults=defs, kw_defaults=[])
		module_node = ast.Module(body=[ast.FunctionDef(
			name=name, args=parameters, body=[ast.Return(
				value=ast.Call(
					func=ast.Name(
						id='_chain', ctx=ast.Load()),
					args=arguments, keywords=[])
				)],
			decorator_list=[])], type_ignores=[])
	else:
		# Python 3 version
		parameters = ast.arguments(args=[ast.arg(arg=arg) for arg in args_with_self],
			kwonlyargs=[], defaults=defs, kw_defaults=[])
		module_node = ast.Module(body=[ast.FunctionDef(
			name=name, args=parameters, body=[ast.Return(
				value=ast.Call(
					func=ast.Name(
						id='_chain', ctx=ast.Load()),
					args=arguments, keywords=[])
				)],
			decorator_list=[])])
	module_node = ast.fix_missing_locations(module_node)
	# compile the ast
	code = compile(module_node, '<string>', 'exec')
	# and eval it in the right context
	globals_ = {'_chain': chain_fn}
	locals_ = dict(('_def{0}'.format(idx), value) for idx, value in enumerate(defaults))
	# locals_['_def0'] = []
	eval(code, globals_, locals_)
	# extract our function from the newly created module
	return locals_[name]

Thanks very much for your time

@addisonElliott
Copy link
Owner

addisonElliott commented Apr 21, 2020

Hm, that's interesting. I wonder if it's related to the issue posted recently #1.

I would definitely accept a patch for this. Add a unit test that fails without the fix and then works with the fix. I would just add a comment for the test that this only means anything on Python 3.8+.

As you can probably tell, this library isn't in wide use so I don't see any reason to remove the Python 2 support, but I won't necessarily go out of my way to support Python 2. My general opinion is that it's been long enough to let it die.

Glad you found the library helpful, and thanks for contributing!

@FredHappyface
Copy link
Contributor Author

I'm honestly not sure 馃

That's great, for the unit tests. Would you be happy for me to test with

BinaryLibrary = namedlist('BinaryLibrary', ['_id', 'name', 'objects'], default=None)

or would that be a little too high level?

As far as python2 goes, I have personally tried to purge every trace of it to no avail as its still in a range of Linux distros. But clearly not everyone wishes to trouble themselves to that extent

Just for reference, here is the stack trace

Traceback (most recent call last):
  File "C:\Users\fredhappyface\Documents\GitHub\LayeredImage\test\test.py", line 13, in <module>
    pdn = layeredimage.io.openLayerImage(THISDIR + "/base24.pdn")
  File "C:\Users\fredhappyface\Documents\GitHub\LayeredImage\layeredimage\io.py", line 30, in openLayerImage
    return openLayer_PDN(file)
  File "C:\Users\fredhappyface\Documents\GitHub\LayeredImage\layeredimage\io.py", line 217, in openLayer_PDN
    from pypdn import read
  File "C:\Python38\lib\site-packages\pypdn\__init__.py", line 2, in <module>
    from pypdn.reader import read, BlendType, Layer, LayeredImage
  File "C:\Python38\lib\site-packages\pypdn\reader.py", line 9, in <module>
    from pypdn.nrbf import NRBF
  File "C:\Python38\lib\site-packages\pypdn\nrbf.py", line 5, in <module>
    from pypdn.util import *
  File "C:\Python38\lib\site-packages\pypdn\util.py", line 101, in <module>
    BinaryLibrary = namedlist('BinaryLibrary', ['_id', 'name', 'objects'], default=None)
  File "C:\Python38\lib\site-packages\pypdn\namedlist.py", line 442, in namedlist
    type_dict = {'__init__': _make_fn('__init__', _nl_init, fields, defaults),
  File "C:\Python38\lib\site-packages\pypdn\namedlist.py", line 183, in _make_fn
    code = compile(module_node, '<string>', 'exec')
TypeError: required field "posonlyargs" missing from arguments

@addisonElliott
Copy link
Owner

馃憤 That's fine with me!

@FredHappyface
Copy link
Contributor Author

That's great. That can be my job tomorrow

addisonElliott added a commit that referenced this issue Apr 22, 2020
Fixes #3 

## Tasks 
- [x] Fix _make_fn for python 3.8+ (pypdn.namedlist._make_fn())
- [x] Test namedList for python 3.8+ (tests.test_namedlist.test_repr_basic_py3_8)

## Differences to proposed solution
- This patch maintains python 2 compatibility 
- Respected source formatting (spaces and indent levels)

Any issues please let me know
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 a pull request may close this issue.

2 participants