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

Implemented Feature::set_field_null #501

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Conversation

metasim
Copy link
Contributor

@metasim metasim commented Dec 20, 2023

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Closes #427
Credit to @nms-scribe

fn test_field_set_null() {
let ds = Dataset::open(fixture("roads.geojson")).unwrap();

for mut layer in ds.layers() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can limit this to a single layer and feature.

@lnicola
Copy link
Member

lnicola commented Dec 20, 2023

I was too afraid to check in #427, looked at the docs now, but still have no idea. Does anyone know the difference between OGR_F_SetFieldNull and OGR_F_UnsetField?

Does the latter actually remove the field value in formats like GeoJSON?

@metasim
Copy link
Contributor Author

metasim commented Dec 20, 2023

Docs:

  • OGR_F_SetFieldNull: Clear a field, marking it as null.
  • OGR_F_UnsetField: Clear a field, marking it as unset.

@lnicola Yeah, I don't understand the semantics either.

Maybe we just mark this as "draft" and come back to it later?

@metasim metasim marked this pull request as draft December 20, 2023 20:15
@lnicola
Copy link
Member

lnicola commented Dec 20, 2023

No, I would have merged it, but unsetting all fields of all features in all layers seems a bit excessive. But it's only 21 features, so feel free to merge 🙂.

@metasim metasim marked this pull request as ready for review December 20, 2023 20:20
@metasim
Copy link
Contributor Author

metasim commented Dec 20, 2023

I simplified the test.

@lnicola lnicola merged commit d1df27d into georust:master Dec 20, 2023
9 checks passed
@metasim metasim deleted the fix/427 branch December 20, 2023 20:32
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.

FR: add wrapper for OGR_F_SetFieldNull
2 participants