Skip to content

FIX: compatibility with SQLAlchemy < 1.4.0#105

Merged
choldgraf merged 2 commits into
executablebooks:mainfrom
DimitriPapadopoulos:declarative_base
Apr 22, 2023
Merged

FIX: compatibility with SQLAlchemy < 1.4.0#105
choldgraf merged 2 commits into
executablebooks:mainfrom
DimitriPapadopoulos:declarative_base

Conversation

@DimitriPapadopoulos
Copy link
Copy Markdown
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Apr 22, 2023

Fixes #93 (comment).

The alternative would be to require sqlalchemy>=1.4.0:

"sqlalchemy>=1.3.12,<3",

@welcome
Copy link
Copy Markdown

welcome Bot commented Apr 22, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2023

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.21%. Comparing base (87e0d74) to head (0dfc913).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
jupyter_cache/cache/db.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
- Coverage   83.30%   83.21%   -0.10%     
==========================================
  Files          20       20              
  Lines        1354     1358       +4     
==========================================
+ Hits         1128     1130       +2     
- Misses        226      228       +2     
Flag Coverage Δ
pytests 83.21% <60.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@choldgraf
Copy link
Copy Markdown
Member

I believe that 0.14 has been out since july of 2021 (https://docs.sqlalchemy.org/en/20/changelog/changelog_14.html#change-1.4.0) so I'd be +1 on just bumping our lower-bound to that version. What do you think?

@DimitriPapadopoulos
Copy link
Copy Markdown
Contributor Author

DimitriPapadopoulos commented Apr 22, 2023

On the other hand, I have read that 1.4 is a "transitional" version (bids-standard/pybids#959 (comment)), and version 2.0 is only a few months old. Not sure if it is a good argument, though:

What’s New in SQLAlchemy 1.4?

One of the primary goals of the 1.4 release is to provide a “transitional” release so that applications may migrate to SQLAlchemy 2.0 gradually. Towards this end, a primary feature in release 1.4 is “2.0 deprecations mode”, which is a series of deprecation warnings that emit against every detectable API pattern which will work differently in version 2.0.

Keeping compatibility with 1.3 only implies a couple additional lines. Why discard that compatibility before it becomes a real issue?

Copy link
Copy Markdown
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

that's a pretty reasonable argument to me - since you've already got this one ready to go, let's just merge it and we can decide to deprecate <1.4 sometime in the future if it really needs to happen.

Thanks for this improvement!

@choldgraf choldgraf changed the title [FIX] compatibility with SQLAlchemy < 1.4.0 FIX: compatibility with SQLAlchemy < 1.4.0 Apr 22, 2023
@choldgraf choldgraf merged commit dfa1daa into executablebooks:main Apr 22, 2023
@welcome
Copy link
Copy Markdown

welcome Bot commented Apr 22, 2023

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@DimitriPapadopoulos DimitriPapadopoulos deleted the declarative_base branch April 22, 2023 18:05
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