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

feat: Support SQLA14 #385

Merged
merged 9 commits into from
Sep 26, 2023
Merged

feat: Support SQLA14 #385

merged 9 commits into from
Sep 26, 2023

Conversation

adhtruong
Copy link
Collaborator

@adhtruong adhtruong commented Sep 22, 2023

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • Pre-Commit Checks were ran and passed
  • Tests were ran and passed

Description

  • Support SQLA1.4

Close Issue(s)

@adhtruong adhtruong changed the title Support sqla14 feat: Support SQLA14 Sep 22, 2023
@adhtruong adhtruong marked this pull request as ready for review September 23, 2023 10:40
@adhtruong adhtruong requested review from a team as code owners September 23, 2023 10:40
@adhtruong
Copy link
Collaborator Author

adhtruong commented Sep 23, 2023

@JacobCoffee this will most likely have conflicts with #384. Happy to help resolve conflicts depending which gets merged first.

.github/workflows/ci.yaml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tests/test_sqlalchemy_factory.py Outdated Show resolved Hide resolved
@JacobCoffee
Copy link
Member

@JacobCoffee this will most likely have conflicts with #384. Happy to help resolve conflicts depending which gets merged first.

It is a race, then 😝

@adhtruong
Copy link
Collaborator Author

adhtruong commented Sep 24, 2023

Updated to

Edit: last point did not work as things are still collected so changed to omitting examples for non-latest

@guacs
Copy link
Member

guacs commented Sep 25, 2023

@adhtruong it seems you lost the race :P Could you resolve the conflicts?

@github-actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/385

@guacs
Copy link
Member

guacs commented Sep 26, 2023

@adhtruong, I tried to run the tests locally with v1 on this branch but I'm getting the following error.

.venv/lib/python3.11/site-packages/_pytest/assertion/rewrite.py:178: in exec_module
    exec(co, module.__dict__)
tests/sqlalchemy_factory/test_sqlalchemy_factory_common.py:123: in <module>
    tuple(SQLAlchemyFactory.get_sqlalchemy_types().keys()),
polyfactory/factories/sqlalchemy_factory.py:80: in get_sqlalchemy_types
    types.TupleType: cls.__faker__.pytuple,
E   AttributeError: module 'sqlalchemy.types' has no attribute 'TupleType'

Are you getting this locally? Or is it some issue with they way I've set it up? The commands I ran were:

source .venv/bin/activate
pdm add sqlalchemy==1.4
pytest tests

@adhtruong
Copy link
Collaborator Author

adhtruong commented Sep 26, 2023

@guacs running that looks like there's two issues here:

  1. pdm add sqlalchemy adds 1.4.0 by default. This is too low a version to support as that hasn't got types.TupleType. Should the patch version be added back to avoid this?
  2. Separate thing noticed from this is that not sure the pipeline installation of pydantic 2 or SQLA1.4 is working. Take this run for example. I'd expect v2 only tests to have been skipped. Happy to fix up the pydantic issue as a separate PR as affecting main right now.

Screenshot 2023-09-26 145725

@guacs
Copy link
Member

guacs commented Sep 26, 2023

@guacs running that looks like there's two issues here:

  1. pdm add sqlalchemy adds 1.4.0 by default. This is too low a version to support as that hasn't got types.TupleType. Should the patch version be added back to avoid this?
  2. Separate thing noticed from this is that not sure the pipeline installation of pydantic 2 or SQLA1.4 is working. Take this run for example. I'd expect v2 only tests to have been skipped. Happy to fix up the pydantic issue as a separate PR as affecting main right now.

Screenshot 2023-09-26 145725

Okay cool. I'm going to merge this for now, but ideally, I'd like it we could support a less restrictive sqlalchemy version. It'd be great if we could support any version of sqlalchemy, but at the very least from v1.4.0 onwards. So I think we should try to see what changes we'd need to make to get it to work on the earlier versions of sqlalchemy.

Also, you're right that there seems to be something wrong with the pipelines, and it'd be a great help if you could look into that :)

@guacs guacs merged commit cabe03c into litestar-org:main Sep 26, 2023
19 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.

Enhancement: Support SQLA v1
3 participants