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

Create fixture for test files of other file types #74

Merged
merged 9 commits into from
Apr 22, 2022

Conversation

theroggy
Copy link
Member

@theroggy theroggy commented Apr 17, 2022

References #73

fgb not possible yet, because the test file contains a combination of polygons and multipolygons
Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @theroggy ! A few changes are needed to get this properly working on multiple extensions.

pyogrio/tests/conftest.py Outdated Show resolved Hide resolved
Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks for continued work on this!

A few minor comments...

pyogrio/tests/conftest.py Outdated Show resolved Hide resolved
pyogrio/tests/conftest.py Outdated Show resolved Hide resolved
pyogrio/tests/conftest.py Show resolved Hide resolved
@brendan-ward
Copy link
Member

combine the best of both worlds

The solution you came up with looks good and addresses my prior concerns over private functions.

create bug issues for these cases

Yes, please!

@theroggy
Copy link
Member Author

theroggy commented Apr 22, 2022

combine the best of both worlds

The solution you came up with looks good and addresses my prior concerns over private functions.

create bug issues for these cases

Yes, please!

It turned out that no new bugs needed to be logged:

  • One issue was already logged: BUG: GDAL does not raise error for invalid column in where expression for GPKG #4
  • Another test failing turned out to be because the fid in GPKG starts at 1 instead of 0, so I changed the test to accomodate for this.
    Do you think it is cleaner to make a set of file types/extensions with 1-based fid instead of zero based?
    Or (not in scope of this pull request), expand on the DRIVERS dict as a dict/class/enum to add these things as properties, e.g.:
    • drivername
    • has_0_based_fid
    • supports_mixed_single_multi (to replace the DRIVERS_NO_MIXED_SINGLE_MULTI dict)
    • supported_write modes? (could reuse the info in _ogr.pyx)
    • ...

@brendan-ward
Copy link
Member

expand on the DRIVERS dict as a dict/class/enum

This seems like a good idea, in a separate PR, since there is a growing amount of properties we have to relate back to specific drivers.

Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks @theroggy !

@brendan-ward brendan-ward merged commit 37e1bfc into geopandas:main Apr 22, 2022
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.

3 participants