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

Fix validation for postgresql+psycopg2 #5

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tonjo
Copy link

@tonjo tonjo commented Apr 16, 2024

Fix validation for postgresql+psycopg2

Fixed validation for postgresql+psycopg2
Fixed validation for postgresql+psycopg2
@adamhmetal
Copy link
Owner

Hello,
Thanks for your pull request.
But red bean python is build in 100% test coverage; before it can be merged there is need to extend tests (and add psycopg2 support if needed).

You can extend:
run_compatibility_tests.py
with


PACKAGES_VERSIONS = {
    (...)
    "psycopg2 (2024)": [
        "SQLAlchemy==2.0.0",
        "psycopg2==2.9.9",
        "PyMySQL==1.0.2",
        "alembic==1.8.0",
        "psycopg==None",
    ],
}


                (...)
                for package_version, packages in PACKAGES_VERSIONS.items():
                    install_dependencies_command = ""
                    for package in packages:
                        if package.endswith("==None"):
                            install_dependencies_command += f"poetry remove {package.split('==')[0]} &&"
                        else:
                            install_dependencies_command += f"poetry add {package} &&"

Currently such tests failed poe test_compatibility as psycopg2 support was not added yet.

If code will be adjusted, and those tests will be fulfilled then pull request can be merged.

Thanks in advance :)

@tonjo
Copy link
Author

tonjo commented Apr 17, 2024

Sorry, I included .vscode/settings by mistake.

@@ -19,6 +19,7 @@
"legacy (2022)": [
"SQLAlchemy==2.0.0",
"psycopg==3.0.18",
"psycopg2==2.9.9",
Copy link
Owner

@adamhmetal adamhmetal Apr 19, 2024

Choose a reason for hiding this comment

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

Unfortanatelly it will not be enough
It will fulfill tests but not because it works.
In fact works because is already psycopg installed in base file
if you remove psycopg you will observe crash.

You can check the way I suggested:


PACKAGES_VERSIONS = {
    (...)
    "psycopg2 (2024)": [
        "SQLAlchemy==2.0.0",
        "psycopg2==2.9.9",
        "PyMySQL==1.0.2",
        "alembic==1.8.0",
        "psycopg==None",
    ],
}


                (...)
                for package_version, packages in PACKAGES_VERSIONS.items():
                    install_dependencies_command = ""
                    for package in packages:
                        if package.endswith("==None"):
                            install_dependencies_command += f"poetry remove {package.split('==')[0]} &&"
                        else:
                            install_dependencies_command += f"poetry add {package} &&"

then you will observe failure, as psycopg2 is not used in such case.

It will need a little more work to make it stable.

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