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

Add support for lambda functions #80

Merged
merged 21 commits into from
Jun 21, 2022
Merged

Add support for lambda functions #80

merged 21 commits into from
Jun 21, 2022

Conversation

andrewcleveland
Copy link
Contributor

@andrewcleveland andrewcleveland commented Apr 5, 2022

Adds to create_function a check if func_name is '<lambda>', and if so builds a lambda expression instead of a regular function definition. This is passed in by @wraps(f) when f is a lambda function.
Without this, create_function builds a function definition 'def <lambda>(...):' which fails to compile.
Also adds a test to verify the above is working.
Resolves #81.

(lambda: None).__name__ is used in place of '<lambda>' in case a different Python implementation uses a different name.
Coverage.py complains about the lambda: None not being run. I don't know if that's an issue or not.

src/makefun/main.py Outdated Show resolved Hide resolved
@smarie
Copy link
Owner

smarie commented Apr 9, 2022

Thanks @andrewcleveland , this is definitely helpful ! I wrote a few comments, mostly details - the principle is good.

@andrewcleveland
Copy link
Contributor Author

andrewcleveland commented Apr 18, 2022

Hi @smarie,
Thanks for taking the time to comment.

Per your note about __code__.co_name, I've taken a different approach and added a new co_name parameter. A lambda is created only in the case that co_name is '<lambda>' or func_impl.__code__.co_name is not valid in a def statement. If that's too much I'll change it back to the original commit with your suggestions added.

@smarie
Copy link
Owner

smarie commented May 3, 2022

Thanks a lot @andrewcleveland ! I have two minor comments, mostly to improve readability/ future maintenance, see above
(by the way sorry for the delay in reviewing !)

@smarie
Copy link
Owner

smarie commented Jun 19, 2022

Hi @andrewcleveland , would you mind updating your PR according to the two comments above, so that we can merge ? Users of pytest-cases will be happy about your mod :) (see smarie/python-pytest-cases#278)

What is missing is just code comments in two sections.
Thanks in advance !

@andrewcleveland
Copy link
Contributor Author

Hi @smarie, sorry for the delay. I did some work on this about a month ago and then kind of put it on the back burner.
I think I've made the comment changes you requested but I may have missed something.
I also added the co_name parameter to some of the other functions for consistency.

src/makefun/main.py Outdated Show resolved Hide resolved
src/makefun/main.py Outdated Show resolved Hide resolved
src/makefun/main.py Outdated Show resolved Hide resolved
src/makefun/main.py Outdated Show resolved Hide resolved
src/makefun/main.py Outdated Show resolved Hide resolved
@smarie
Copy link
Owner

smarie commented Jun 21, 2022

Thanks @andrewcleveland ! I had a few other comments, hopefully easy to cope with

andrewcleveland and others added 5 commits June 21, 2022 03:53
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
@andrewcleveland
Copy link
Contributor Author

andrewcleveland commented Jun 21, 2022

Hi @smarie, I made the changes you suggested.
I changed the adding a space to func_signature_str to a check that uses a different body string for a lambda with/without arguments. It seems a bit easier to read.

Moving the conditional is_identifier import to the start of main.py seems to create a circular dependency.
The following error happens:

.nox/tests-2-7/lib/python2.7/site-packages/makefun/main.py:22: in <module>
     from makefun._main_legacy_py import is_identifier
 .nox/tests-2-7/lib/python2.7/site-packages/makefun/_main_legacy_py.py:8: in <module>
     from makefun.main import wraps
 E   ImportError: cannot import name wraps

I moved the import to the bottom of main.py, but I'm not sure if that's the best way to resolve it.

@smarie
Copy link
Owner

smarie commented Jun 21, 2022

Thanks @andrewcleveland !
Concerning the is_identifier, can you do one of the two options below:

  • option1: move the def is_identifier back to the main.py, in the else branch (basically replace the from makefun._legacy import is_identifier by the def
  • option 2: in the legacy module, move the from makefun.main import wraps into the body of the function where it is used. I like this option less, to be honest

that way, the if/else can be moved to the top just after the imports, and there is no circular dependency anymore.

Thanks!

@andrewcleveland
Copy link
Contributor Author

@smarie, option 1 is done.

@smarie smarie merged commit 947b06c into smarie:main Jun 21, 2022
@smarie
Copy link
Owner

smarie commented Jun 21, 2022

All set, thanks a lot @andrewcleveland for your patience !
I'll tag and release asap

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.

wraps() fails when wrapped function is a lambda
2 participants