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

Incorporate import_expression logic into jishaku. #231

Conversation

Sachaa-Thanasius
Copy link

@Sachaa-Thanasius Sachaa-Thanasius commented Apr 14, 2024

Rationale

import_expression is effectively unmaintained and isn't functional on python3.12 (according the issue linked below). The new logic should be functional on 3.8+.

Closes #228.

Summary of changes made

Adds a token stream transformer and AST transformer and incorporates them into a parse function with the same signature as ast.parse to achieve a similar end result to import_expression.parse. Also added tests, mostly sourced from import_expression and modified to account for the different and much smaller API surface.

Checklist

  • This PR changes the jishaku module/cog codebase
    • These changes add new functionality to the module/cog
    • These changes fix an issue or bug in the module/cog
    • I have tested that these changes work on a production bot codebase
    • I have tested these changes against the CI/CD test suite
    • I have updated the documentation to reflect these changes
  • This PR changes the CI/CD test suite
    • I have tested my suite changes are well-formed (all tests can be discovered)
    • These changes adjust existing test cases
    • These changes add new test cases
  • This PR changes prose (such as the documentation, README or other Markdown/RST documents)
    • I have proofread my changes for grammar and spelling issues
    • I have tested that any changes regarding Markdown/RST syntax result in a well formed document

- Modify logic from Sachaa-Thanasius/experimental.
- Copy modified tests.
- Substitute import_expression.parse with inline_import.parse.
- Remove IMPORTER.
- Fix test break, since `tokenize.tokenize` has buggy behavior that wasn't backported.
 - See python/cpython#79288 and python/cpython#88833.
- Adjust typing using so everything from typing is prepended with `typing.`.
@Sachaa-Thanasius Sachaa-Thanasius marked this pull request as ready for review April 14, 2024 22:04
@Sachaa-Thanasius
Copy link
Author

Sachaa-Thanasius commented Apr 14, 2024

Regarding the remaining points:

  • I have a bot, but depending on the meaning of "production", it might not fill that criteria? It's a small one just for running jishaku, and it's currently using my branch on 3.12. The import syntax seems to be working fine so far.
    • EDIT: See the attached image below.
  • For tests, I hope running python -m pytest for py3.8-3.12 locally will be enough on my end for now.
  • For documentation, I am currently imagining a single bullet point in the changelog for 2.5.3 that says something along the lines of, "Implemented import_expressions functionality within Jishaku and fixed it to work on 3.8 - 3.12". Would this suffice?

@Gorialis

@Sachaa-Thanasius
Copy link
Author

Sachaa-Thanasius commented Apr 15, 2024

On a tangential note: it was mentioned in the related issue that the inline import feature should pass "the litmus test" on 3.8-3.12, but in the overall tracking issue for the next release (#212), it was also mentioned that jishaku would get a bump to 3.10. Is the latter still true, and if so, do I only have to care about this being compatible with 3.10+?

- `typing_extensions` is a direct dependency, so using it directly is fine.
- `copy_annotations()` was using functools.wraps with the wrong function as the wrapper.
Copy link
Contributor

@tuna2134 tuna2134 left a comment

Choose a reason for hiding this comment

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

It's looks like ok

@Sachaa-Thanasius
Copy link
Author

Sachaa-Thanasius commented May 26, 2024

Since ioistired is back, I'll close this PR once upstream has been fixed.

ioistired added a commit to ioistired/jishaku that referenced this pull request May 27, 2024
ioistired added a commit to ioistired/jishaku that referenced this pull request May 27, 2024
scarletcafe pushed a commit that referenced this pull request May 28, 2024
@Sachaa-Thanasius Sachaa-Thanasius deleted the incorporate-import-expression branch May 28, 2024 22:33
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.

[Tracking] import_expression doesn't function correctly in 3.12
2 participants