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

Handle setting geometry column name for GDAL 3.6+ #123

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

yohancabion
Copy link
Contributor

No description provided.

@coveralls
Copy link
Collaborator

coveralls commented Mar 20, 2024

Pull Request Test Coverage Report for Build 8388745198

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 98.499%

Totals Coverage Status
Change from base Build 7272942237: 0.007%
Covered Lines: 3543
Relevant Lines: 3597

💛 - Coveralls

@yohancabion yohancabion force-pushed the column-name-3-6 branch 4 times, most recently from 61859f8 to 79cc9be Compare March 21, 2024 10:15
godal.go Outdated
@@ -2861,6 +2861,7 @@ func (f *Feature) SetGeometry(geom *Geometry, opts ...SetGeometryOption) error {
return cgc.close()
}

// Deprecated: when running with GDAL 3.6+, you may use SetGeometryColumnName on Layer
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking we should change this method's signature to return an error, and explicitely return a go error without going into cgo/gdal for version>=3.6

Copy link
Member

Choose a reason for hiding this comment

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

or for version>3.9 actually no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments and method signature changed. Tell me if it's ok so I squash commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we look at the coverage, I wonder if we may create a cpp function so we check version in cpp code, like for other functions.

Copy link
Member

Choose a reason for hiding this comment

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

As you prefer. I am not very concerned by the coverage percent for this particular issue. If you were to move it all to cpp (which is effectively cleaner and a bit more in line with what we do elsewhere in godal), your coverage would still not be ideal as it would not be possible to write a test handling both the error and no-error cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New changes done. When moving to cpp, as errors are handled in a proper way, we don't even need to check version.

@tbonfort
Copy link
Member

LGTM

@yohancabion yohancabion marked this pull request as ready for review March 22, 2024 10:15
@tbonfort tbonfort merged commit cf193d9 into airbusgeo:main Mar 22, 2024
19 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.

3 participants