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

Move to SQLAlchemy 2 #187

Draft
wants to merge 40 commits into
base: master
Choose a base branch
from
Draft

Move to SQLAlchemy 2 #187

wants to merge 40 commits into from

Conversation

RudolfCardinal
Copy link
Collaborator

No description provided.

@RudolfCardinal RudolfCardinal marked this pull request as draft January 8, 2025 12:15
@RudolfCardinal
Copy link
Collaborator Author

Depends on RudolfCardinal/pythonlib#23, and (for proper installation/distribution), on pushing cardinal_pythonlib==2.0.0 to PyPI.

@martinburchell
Copy link
Collaborator

martinburchell commented Jan 8, 2025

Depends on RudolfCardinal/pythonlib#23, and (for proper installation/distribution), on pushing cardinal_pythonlib==2.0.0 to PyPI.

Until then you could pin cardinal_pythonlib to a git branch in setup.py e.g.:

"cardinal_pythonlib @ git+https://github.com/RudolfCardinal/pythonlib@sqlalchemy2#egg=cardinal_pythonlib-2.0.0-rc1"

I have not tested this!

@RudolfCardinal
Copy link
Collaborator Author

boo, "Dependency is not satisfiable: libssl1.1"

Copy link
Collaborator

@martinburchell martinburchell left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've pushed a few more changes including a test fix and a pre-commit hook to check for blanket noqas.

Before merging, there's the zero length upsert_test_1.sql, which I think must have been checked in accidentally. It would be good also to test the changes from #180 on the CPFT server.

We'll also need to release the new cardinal_pythonlib and update setup.py

Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidentally checked in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! Sorry. Removed.

configtext = self._mk_nlp_config()
log.debug(configtext)
f.write(configtext)
# import pdb; pdb.set_trace()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# import pdb; pdb.set_trace()

@RudolfCardinal
Copy link
Collaborator Author

Thanks - checks in progress but have incorporated suggestions above. But will wait as you suggest until CPFT tests complete; just say when (or feel free).

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