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

Clean up exec and globals #1761

Merged
merged 3 commits into from
Dec 23, 2024

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Aug 26, 2024

Using exec and globals can sometimes obscure what code is doing.

The last commit also fixes bug #13347.

@QuLogic QuLogic force-pushed the less-exec branch 2 times, most recently from cf88d09 to f60f8dc Compare August 26, 2024 08:26
result = {}
exec(s, globals(), result)
return result["fn"]
except SyntaxError:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, since the string above only defines a function, I can't see how this could fail in a way other than a syntax error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS, this file could be written in a way that doesn't use exec with some lambdas, but as it seemed that this was done for speed, I'm not sure if I should change that without having any benchmarks available. So I wonder if there are any benchmarks for this bit of code?

Comment on lines +549 to +551
olddad,
yngmom,
yngdad,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two examples of undefined variables that happened because globals() modifications set them, instead of passing them as parameters.

result = {}
exec(s, globals(), result)
return result["fn"]
except SyntaxError:
Copy link

Choose a reason for hiding this comment

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

Wrong format strings produce TypeErrors:

>>> '' % 'a'
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    '' % 'a'
    ~~~^~~~~
TypeError: not all arguments converted during string formatting
>>> '%x' % 'a'
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    '%x' % 'a'
    ~~~~~^~~~~
TypeError: %x format: an integer is required, not str
>>> '%s %s' % 'a'
Traceback (most recent call last):
  File "<python-input-2>", line 1, in <module>
    '%s %s' % 'a'
    ~~~~~~~~^~~~~
TypeError: not enough arguments for format string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Format strings are only evaluated on the line defining s earlier, or within fn itself, but not at the outer level at which the exec is run.

All the examples you've given pass when wrapped in def fn():.

Note, fn is not called here, but cached for use in the callers of this function.

Copy link

Choose a reason for hiding this comment

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

Alright then, sorry for the noise.

@usdanskys
Copy link

Applied all changes to the individual files in my Fedora 41 install. Seems to work!

@justbyitself
Copy link

justbyitself commented Dec 23, 2024

Please, merge this pull request. When you update to Python 3.13, you'll get ugly errors everywhere. This is a critical patch, not just a code quality enhancement.
Tested on Arch Linux and it works. Thanks @QuLogic.

Copy link
Member

@dsblank dsblank left a comment

Choose a reason for hiding this comment

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

Changes look good. Nice cleanup!

@dsblank dsblank added the bug label Dec 23, 2024
@dsblank
Copy link
Member

dsblank commented Dec 23, 2024

@Nick-Hall, this is a bug fix for Python 3.13, and some nice cleanup.

Especially in the latter case of the `Verify` plugin, this exposed that
some methods were missing parameters.
As of PEP558, `locals()` is not populated by `exec()`. This change means
that this call is broken on Python 3.13.
@Nick-Hall Nick-Hall changed the base branch from master to maintenance/gramps52 December 23, 2024 23:02
@Nick-Hall
Copy link
Member

Rebased onto the gramps52 branch.

@Nick-Hall Nick-Hall merged commit 4ff81ad into gramps-project:maintenance/gramps52 Dec 23, 2024
@QuLogic QuLogic deleted the less-exec branch December 24, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants