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

Pump Version python to 3.12, 3.13. Allow pip install for python 3.12, 3.13 #210

Merged
merged 16 commits into from
Dec 20, 2024

Conversation

maycuatroi
Copy link
Contributor

Overview

Update file pyproject.toml to enable pip install for python 3.12

References

Example workbook work perfect when I pip install on python 3.12
image

@maycuatroi maycuatroi requested a review from odashi as a code owner October 8, 2024 12:32
Copy link

google-cla bot commented Oct 8, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@maycuatroi
Copy link
Contributor Author

It's strange, I didn't make any changes to the Python code, yet it's causing CI errors with mypy.
@odashi-san, Do you think I should fix these CI errors?

@odashi
Copy link
Collaborator

odashi commented Oct 9, 2024

@maycuatroi
It looks the errors occurred around obsolete syntax rules that the new version of mypy (after 1.9) removed its support.
For now, we can set mypy version to up to 1.8 to bypass the error, though it's better to remove obsolete code entirely from latexify.

- Improve CI libraries
@maycuatroi
Copy link
Contributor Author

maycuatroi commented Oct 9, 2024

@odashi -san. I fixed CI to the last version of mypy and black. Please approve the CI scan

@odashi
Copy link
Collaborator

odashi commented Oct 9, 2024

@maycuatroi Thanks!

I think you tweaked some code blocked by version switches, e.g.,

  • if blocks conditioned by if sys.version_info.minor < blah:
  • test blocks conditioned by @test_utils.require_at_most(blah)

These blocks are activated while the library is running on a conditioned version of Python, and the code block must follow the correct syntax on the corresponding version.

I guessed that this is a good time to drop support for legacy (EOL-ed) versions such as 3.7 and 3.8, by simply removing corresponding blocks from the library.
Sorry for bothering, but I'd quite appreciated it if you could take some additional efforts.

- Remove ORL python 3.8, 3.9
- Fix CI unit-test
@maycuatroi
Copy link
Contributor Author

@odashi -san
I've checked and fixed the unit tests for the CI accordingly. Here's what I did:

  • Removed support for Python 3.7 and 3.8 (since they've reached end-of-life).
  • Tested with Python 3.13 – everything worked great!

I really enjoy using this library, and I appreciate your professionalism.

To ensure everything works across multiple Python versions, I also created a custom Docker Compose setup for running the CI tests on various versions 😄 (I'm not commit it yet):

version: "3.8"

services:
  unit-tests:
    image: python:3.13
    environment:
      - PYTHONPATH=src
    volumes:
      - .:/app  # This will copy all files in the current directory into the /app directory in the container
    working_dir: /app
    command: >
      bash -c "
      python -m pip install --upgrade pip &&
      python -m pip install -e '.[dev]' &&
      python -m pytest src
      "

  black:
    image: python:3.13
    volumes:
      - .:/app  # Mounting the project directory
    working_dir: /app
    command: >
      bash -c "
      python -m pip install --upgrade pip &&
      python -m pip install black &&
      python -m black -v --check src
      "

  flake8:
    image: python:3.13
    volumes:
      - .:/app  # Mounting the project directory
    working_dir: /app
    command: >
      bash -c "
      python -m pip install --upgrade pip &&
      python -m pip install pyproject-flake8 &&
      pflake8 -v src
      "

  isort:
    image: python:3.13
    volumes:
      - .:/app  # Mounting the project directory
    working_dir: /app
    command: >
      bash -c "
      python -m pip install --upgrade pip &&
      python -m pip install isort &&
      python -m isort --check src
      "

  mypy:
    image: python:3.13
    volumes:
      - .:/app  # Mounting the project directory
    working_dir: /app
    command: >
      bash -c "
      python -m pip install --upgrade pip &&
      python -m pip install '.[mypy]' &&
      python -m mypy src
      "

@maycuatroi maycuatroi changed the title Pump Version python to 3.12. Allow pip install for python 3.12 Pump Version python to 3.12, 3.13. Allow pip install for python 3.12, 3.13 Oct 9, 2024
Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

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

Great work!

src/latexify/test_utils.py Outdated Show resolved Hide resolved
src/latexify/test_utils.py Outdated Show resolved Hide resolved
src/latexify/ast_utils.py Outdated Show resolved Hide resolved
src/latexify/ast_utils_test.py Outdated Show resolved Hide resolved
src/latexify/transformers/identifier_replacer.py Outdated Show resolved Hide resolved
@maycuatroi
Copy link
Contributor Author

@odashi -san
I Create new function ast_function_def to handle type_params for python 3.12 and I fixed the flake8 CI issue. Please help me review and approve workflow 🙏

@maycuatroi
Copy link
Contributor Author

@odashi -san. I think things look good 😄

Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

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

Cool, basically everything looks good. Comments are almost based on coding conventions.

src/latexify/ast_utils.py Outdated Show resolved Hide resolved
src/latexify/ast_utils.py Outdated Show resolved Hide resolved
src/latexify/ast_utils_test.py Show resolved Hide resolved
src/latexify/parser_test.py Outdated Show resolved Hide resolved
@maycuatroi
Copy link
Contributor Author

@odashi -san, please approve the workflow, I fixed the isort issue 😄

@maycuatroi
Copy link
Contributor Author

@odashi -san, please help me review. I updated follow your comment 😃

Copy link
Collaborator

@odashi odashi 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 almost done I think, but there are still some little comments (maybe these are the last ones)

(ast.Constant(value=None), True),
(ast.Constant(value=123), True),
(ast.Constant(value="baz"), True),
(ast.Expr(value=ast.Constant(value=456)), False),
(ast.Global(names=["qux"]), False),
],
)
def test_is_constant_legacy(value: ast.AST, expected: bool) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks there exists some test cases from that only @test_utils.require_at_most(7) was removed like this. Could you check the changes entirely and remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The decorator (@test_utils.require_at_most(7)) was previously used to indicate that the test case would run on Python versions >= 3.7. So, even when we were on Python 3.9, the test case was still executed. Now that we've upgraded to at least Python 3.9, the test case should still run as intended. I believe it’s best to keep the test case as it is.

Copy link
Collaborator

@odashi odashi Oct 25, 2024

Choose a reason for hiding this comment

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

require_at_most(k) requires versions equal or less than 3.k. the decorated function does nothing (skip everything) otherwise.

if sys.version_info.minor > minor:
return

Comment on lines +153 to +163
name,
args,
body,
decorator_list,
returns=None,
type_comment=None,
type_params=None,
lineno=None,
col_offset=None,
end_lineno=None,
end_col_offset=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add type hints.

(ast.Constant(value=None), False),
(ast.Constant(value=123), False),
(ast.Constant(value="baz"), True),
(ast.Expr(value=ast.Constant(value=456)), False),
(ast.Global(names=["qux"]), False),
],
)
def test_is_str_legacy(value: ast.AST, expected: bool) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test case as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same with decorator @test_utils.require_at_most(7)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, removing every test function with decoration @test_utils.require_at_most(7) should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@odashi Yes, I removed all the test function decorated with @test_utils.require_at_most(7)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@odashi please check and approve the Checks :D

@odashi odashi mentioned this pull request Dec 16, 2024
Copy link
Collaborator

@odashi odashi left a comment

Choose a reason for hiding this comment

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

LGTM

@odashi odashi merged commit f9c0797 into google:main Dec 20, 2024
10 checks passed
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.

2 participants