8000 feat: 3.12 support for the PositionNodeFinder by 15r10nk · Pull Request #71 · alexmojaki/executing · GitHub
[go: up one dir, main page]
Skip to content

feat: 3.12 support for the PositionNodeFinder #71

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 24 commits into from
Sep 24, 2023
Merged

feat: 3.12 support for the PositionNodeFinder #71

merged 24 commits into from
Sep 24, 2023

Conversation

15r10nk
Copy link
Collaborator
@15r10nk 15r10nk commented May 18, 2023 •
edited
Loading

fix #70

this merge-request adds support for the current 3.12.0a7

Unfortunately I had to skip test_module_files for the SentinelNodeFinder, because there where random failures.

@alexmojaki we should discuss how we want to proceed with this issue. I will try to fixe some issues with the node finder to get a better understanding. I hope it will not get to complicated.

@15r10nk 15r10nk force-pushed the 3.12 branch 3 times, most recently from fe7478e to 748a3c0 Compare May 19, 2023 07:05
@15r10nk 15r10nk mentioned this pull request May 19, 2023
Closed
@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 9, 2023 •
edited
Loading

reported the following issue python/cpython#109195.

source positions of LOAD_GLOBAL super includes the parentheses of super()

@alexmojaki
Copy link
Owner

Unfortunately I had to skip test_module_files for the SentinelNodeFinder, because there where random failures.

I tried taking a look in #73. At least in 3.9, test_module_files seems to have passed, but test_sample_files failed. I don't know if that's related to what you saw, I haven't investigated.

I also decided to drop support for Python < 3.8. The same is happening in gristlabs/asttokens#117 and we've suffered enough. Hopefully that helps.

@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 10, 2023

hmm I looked also into these issues and fixed some of theme. However it seems that I forgot to push this branch.
I pushed it now #74.

I don't know if that's related to what you saw, I haven't investigated.

The thing is that this was some time ago and I can not remember if I was able to solve all issues. But if you want do deprecate python < 3.8 ❤️ #64 might be also interesting for you. This is a complete new NodeFinder implementation for >= 3.8 which works similar to the PositionNodeFinder. But I would like to review my own code first before you go into a deeper review. The algo is quite complicated and some documentation could improve the readability.

I work currently on pysource-codegen to generate really crazy python code. I used it so far to find several bugs in python 3.12 itself (mostly for the new type parameter syntax).

My (and propably your to) limiting factor is time 😄
I can continue to work on some executing issues when I'm done reporting the cpython 3.12 issues.

It would be really useful if you could tell me what you would prefer to be done first.

@alexmojaki
Copy link
Owner

The main priority is to release a version of executing that works with 3.12 before the final release, which is planned for 2nd October (https://peps.python.org/pep-0693/). If some odd new test failures in older versions have appeared, it's fine to do the minimum work required (e.g. skipping certain files) to make the build pass, as long as nothing becomes broken in a new way.

Thank you so much for all your help ❤️

@alexmojaki
Copy link
Owner

I have time to help at the moment, let me know if there's something that makes sense for me to look at without duplicating work.

@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 16, 2023

Good news for you, I think the hardest part is over.
I was able to check most/all python files on my PC. I will go one more time through the commits and check if some things have changed since I tested the first alpha.

I'm currently on a short holiday trip with my family and will continue to work on this merge request next week. My best guess is that you will be able to review the code at the end of next week.

Thank you for your offer to help. Have a nice weekend. 😉

@15r10nk 15r10nk force-pushed the 3.12 branch 4 times, most recently from 8f0a6a5 to 785a36d Compare September 17, 2023 19:07
@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 17, 2023

hi @alexmojaki, maybe you have a tip for me for the following problem.

https://github.com/alexmojaki/executing/actions/runs/6215351046/job/16868163417

It looks for me like the ast module misses some type information for 3.12. Do you have any tip how I could solve this issue?

@alexmojaki
Copy link
Owner

Just disable mypy for 3.12 for now. Looks like they're not ready either: python/mypy#15277

@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 18, 2023

next problem @alexmojaki.

https://github.com/alexmojaki/executing/actions/runs/6215643716/job/16868740630

the tests for 3.9 fail because the sample_results have changed. The problem is that I get the same error for the master branch (also for different cpython minor versions).

I will try to solve this problem, but it would be easier when I would know why it happen.

@alexmojaki
Copy link
Owner

asttokens recently started supporting f-strings better, which resulted in 45d8ca2

But then that changed for Python 3.9: gristlabs/asttokens#104

So nothing is wrong in executing.

@alexmojaki
Copy link
Owner

You can update the samples with FIX_EXECUTING_TESTS=1 EXECUTING_SLOW_TESTS=1 tox -e py39, and those lines will go back to empty strings. Or you could add these lines to test_main.py:

if sys.version_info[:2] == (3, 9):
    # https://github.com/alexmojaki/executing/pull/71#issuecomment-1723634310
    import asttokens.util
    asttokens.util.fstring_positions_work = lambda: True

@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 18, 2023

thank you, you saved me a lot of time.

@15r10nk 15r10nk force-pushed the 3.12 branch 2 times, most recently from 22caeb7 to 81db419 Compare September 18, 2023 18:07
@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 22, 2023

hi @alexmojaki, I think it is time for a review.

alexmojaki
alexmojaki reviewed Sep 22, 2023
View reviewed changes
alexmojaki
alexmojaki reviewed Sep 22, 2023
View reviewed changes
alexmojaki
alexmojaki reviewed Sep 22, 2023
View reviewed changes
alexmojaki
alexmojaki reviewed Sep 22, 2023
View reviewed changes
alexmojaki
alexmojaki reviewed Sep 24, 2023
View reviewed changes
alexmojaki
alexmojaki reviewed Sep 24, 2023
View reviewed changes
alexmojaki
alexmojaki approved these changes Sep 24, 2023
View reviewed changes
Copy link
Owner
@alexmojaki alexmojaki left a comment

Choose a reason for hiding this comment

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

LGTM! Think it's ready?

@alexmojaki alexmojaki marked this pull request as ready for review September 24, 2023 11:18
@15r10nk
Copy link
Collaborator Author
15r10nk commented Sep 24, 2023 •
edited
Loading

I have added support for __iter__ and __next__. It is ready from my side if the next ci-run passes without errors.

alexmojaki
alexmojaki reviewed Sep 24, 2023
View reviewed changes
@alexmojaki alexmojaki merged commit 02ebf5b into alexmojaki:master Sep 24, 2023
@alexmojaki
Copy link
Owner

Released v2.0.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexmojaki alexmojaki alexmojaki approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Various tests fail with Python 3.12
2 participants
@15r10nk @alexmojaki
0