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

PostgreSQL Provider: Fix M detection #56199

Merged
merged 2 commits into from Feb 6, 2024

Conversation

lbartoletti
Copy link
Member

Description

Unlike Geometry(Z), GeometryM contains already M suffix. It's useless to add M suffix here, since it will add a new name and the geometry type name will be something like GeometryMM.

Fixes #55223

@github-actions github-actions bot added this to the 3.36.0 milestone Feb 6, 2024
@lbartoletti
Copy link
Member Author

with this "fix":

image

cc @Michael-cd30

Unlike Geometry(Z), GeometryM contains already M suffix.
It's useless to add M suffix here, since it will add a new name
and the geometry type name will be something like GeometryMM.

Fixes qgis#55223
@lbartoletti
Copy link
Member Author

and :

image

@lbartoletti lbartoletti self-assigned this Feb 6, 2024
Copy link
Contributor

@elpaso elpaso left a comment

Choose a reason for hiding this comment

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

LGTM
Any chance to have a test?

// It's useless to add M suffix here,
// since it will add a new name and the geometry type name will be something like GeometryMM.
// see: https://github.com/qgis/QGIS/issues/55223
//typeString.append( 'M' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid introducing commented-out code here? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I wasn't sure about my fix, and it still seems like something that deserves an explanation, I preferred to add my comment and leave the code as a comment. Do you agree, if I leave my comment, but delete the "faulty" line?

Copy link

github-actions bot commented Feb 6, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit c3f763d)

@lbartoletti
Copy link
Member Author

Any chance to have a test?

@elpaso I confess I have no idea how to test it :/

@elpaso
Copy link
Contributor

elpaso commented Feb 6, 2024

Any chance to have a test?

@elpaso I confess I have no idea how to test it :/

Create the test table test_linestringm then something like:

md = QgsProviderRegistry.instance().providerMetadata('postgres')
geom_types = [t.geometryColumnTypes()[0] for t in md.createConnection('dbname=qgis_test port=5433', {}).tables() if t.tableName() == 'test_linestringm']

# Expected:
[<QgsAbstractDatabaseProviderConnection.TableProperty.GeometryColumnType: 'LineString, EPSG:2154'>,
<QgsAbstractDatabaseProviderConnection.TableProperty.GeometryColumnType: 'LineStringM, EPSG:2154'>]

See here for examples: https://github.com/qgis/QGIS/blob/master/tests/src/python/test_qgsproviderconnection_postgres.py#L386

@lbartoletti
Copy link
Member Author

Any chance to have a test?

@elpaso I confess I have no idea how to test it :/

Create the test table test_linestringm then something like:

md = QgsProviderRegistry.instance().providerMetadata('postgres')
geom_types = [t.geometryColumnTypes()[0] for t in md.createConnection('dbname=qgis_test port=5433', {}).tables() if t.tableName() == 'test_linestringm']

# Expected:
[<QgsAbstractDatabaseProviderConnection.TableProperty.GeometryColumnType: 'LineString, EPSG:2154'>,
<QgsAbstractDatabaseProviderConnection.TableProperty.GeometryColumnType: 'LineStringM, EPSG:2154'>]

See here for examples: https://github.com/qgis/QGIS/blob/master/tests/src/python/test_qgsproviderconnection_postgres.py#L386

thanks!

@lbartoletti lbartoletti enabled auto-merge (rebase) February 6, 2024 15:28
@lbartoletti lbartoletti merged commit 58c1da3 into qgis:master Feb 6, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to add layers with M (measure) geometries from PostGIS
3 participants