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

BUG: fix handling of hashes in file paths #3280

Merged
merged 5 commits into from
May 15, 2024

Conversation

m-richards
Copy link
Member

@m-richards m-richards commented May 8, 2024

Closes #3279.

This is an upstream bug in pyogrio is well. It may also be upstream in fiona, but haven't check that yet.

Test failure is for dev and unrelated.

There's no changelog entry as this modifies #3232 which is also unreleased (well it's in alpha 1. It seems that we've merged changelog entries after the alpha into the alpha 1 section, so I assume we will actually not keep the alpha section, we'll just have a combined changelog for 1.0)

@@ -694,6 +695,17 @@ def test_allow_legacy_gdal_path(engine, nybb_filename):
assert isinstance(gdf, geopandas.GeoDataFrame)


def test_read_file_with_hash_in_path(engine, nybb_filename, tmp_path):
if engine == "pyogrio":
pytest.xfail("upstream bug")
Copy link
Member Author

Choose a reason for hiding this comment

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

we can turn this into a conditional skip once geopandas/pyogrio#412 is in.

Copy link
Member

Choose a reason for hiding this comment

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

Can be updated now

@m-richards m-richards changed the title BUG: fix handling of hashes in file schemes BUG: fix handling of hashes in file paths May 9, 2024
@jorisvandenbossche
Copy link
Member

There's no changelog entry as this modifies #3232 which is also unreleased

So this was essentially a regression on main (caused by #3232), but with released geopandas this was working fine?

@scazz010
Copy link

scazz010 commented May 13, 2024

So this was essentially a regression on main (caused by #3232), but with released geopandas this was working fine?

This bug is present in released geopandas 0.14.4

@m-richards
Copy link
Member Author

m-richards commented May 13, 2024

This bug is present in released geopandas 0.14.4

Ah good catch, it was backported. To your question @jorisvandenbossche I wasn't 100% sure above, but it looks like this was fine in previous versions with fiona, as it checks if a path string conforms to be below before urlparse is hit (snippet in parse_path):

    elif re.match("^[a-z0-9\\+]*://", path):
        parts = urlparse(path)

I suppose there's the question of if there should be an equivalent check in the pyogrio code / the port of the pyogrio code in geopandas used for the fiona 1.10 support path.

CHANGELOG.md Outdated
@@ -1,6 +1,6 @@
# Changelog

## Version 1.0.0-alpha1 (Apr 13, 2024)
## Version 1.0.0
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm assuming we deliberately chose to mix new entries into the changelog for alpha1 to create a consolidated changelog for the final 1.0 release, so I updated the header accordingly.

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 a5a9a5b into geopandas:main May 15, 2024
19 of 20 checks passed
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.

BUG: Unable to open files with fiona where filepath contains a # character
4 participants