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

MAINT: dealing with sqlalchemy & geomet #592

Merged
merged 11 commits into from
Oct 22, 2023

Conversation

jGaboardi
Copy link
Member

This PR resolves #588

@jGaboardi jGaboardi self-assigned this Oct 21, 2023
@jGaboardi jGaboardi added maintenance dependencies Pull requests that update a dependency file labels Oct 21, 2023
@jGaboardi jGaboardi requested a review from martinfleis October 21, 2023 20:45
@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

Merging #592 (433920a) into main (9289ba4) will increase coverage by 0.3%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #592     +/-   ##
=======================================
+ Coverage   84.2%   84.5%   +0.3%     
=======================================
  Files        128     128             
  Lines      15080   15069     -11     
=======================================
+ Hits       12698   12734     +36     
+ Misses      2382    2335     -47     
Files Coverage Δ
libpysal/io/iohandlers/db.py 88.7% <100.0%> (+46.7%) ⬆️
libpysal/io/iohandlers/tests/test_db.py 91.9% <100.0%> (+41.9%) ⬆️
libpysal/weights/tests/test_user.py 91.3% <100.0%> (+0.4%) ⬆️

@jGaboardi jGaboardi changed the title MAINT: dealing with sqlalchemy & geomet` MAINT: dealing with sqlalchemy & geomet Oct 21, 2023
@jGaboardi jGaboardi changed the title MAINT: dealing with sqlalchemy & geomet MAINT: dealing with sqlalchemy & geomet Oct 21, 2023
@jGaboardi
Copy link
Member Author

Seems to be an OS-specific failure:

FAILED libpysal/io/iohandlers/tests/test_db.py::Test_sqlite_reader::test_deserialize - 
    PermissionError: [WinError 32] The process cannot access the file because it is 
    being used by another process: 'test.db'

@jGaboardi
Copy link
Member Author

jGaboardi commented Oct 21, 2023

This is actually happening in the tearDown portion.

def tearDown(self):
>       os.remove("test.db")
E       PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'test.db'

Probably can simply move os.remove("test.db") into the actual test.

libpysal/io/iohandlers/tests/test_db.py Outdated Show resolved Hide resolved
libpysal/io/iohandlers/tests/test_db.py Outdated Show resolved Hide resolved
@jGaboardi
Copy link
Member Author

Welp, not really sure why we are still seeing the PermissionError on Windows. Gave it go trying to resolve correctly. Unless there are other ideas, should we either skip of xfail for Windows?

@martinfleis
Copy link
Member

Skip it on win. I suppose that this code will eventually go anyway. You can probably replace it with geopandas read_sql or read SQL with GDAL via pyogrio or fiona, all of which are way more maintained than our io module.

@jGaboardi
Copy link
Member Author

Skip it on win. I suppose that this code will eventually go anyway. You can probably replace it with geopandas read_sql or read SQL with GDAL via pyogrio or fiona, all of which are way more maintained than our io module.

I'll just implement the skip for now. Better to do baby steps.

@jGaboardi jGaboardi requested a review from martinfleis October 22, 2023 18:28
@jGaboardi
Copy link
Member Author

We've got 100% green CI!

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Thanks!

@martinfleis martinfleis merged commit 9de3108 into pysal:main Oct 22, 2023
10 checks passed
@jGaboardi jGaboardi deleted the sql_geomet_issue branch October 22, 2023 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sqlalchemy and geomet
2 participants