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

Add SpatialRef::to_projjson() #389

Merged
merged 2 commits into from
Apr 24, 2023
Merged

Conversation

ttencate
Copy link
Contributor

@ttencate ttencate commented Apr 24, 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.

@ttencate ttencate force-pushed the feature/srs_to_projjson branch from b85800a to c39baff Compare April 24, 2023 10:36
} else {
Ok(_string(c_projjsonstr))
};
unsafe { gdal_sys::VSIFree(c_projjsonstr.cast::<std::ffi::c_void>()) };
Copy link
Member

Choose a reason for hiding this comment

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

I don't even know any more which one of .cast(), .cast<c_void>() or as _ is preferred 😃. At least this is consistent with the rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's mostly a copy/paste... The entire lib could do with some helper functions/macros to streamline stuff like this.

@@ -506,6 +506,22 @@ impl SpatialRef {
res
}

pub fn to_projjson(&self) -> Result<String> {
Copy link
Member

Choose a reason for hiding this comment

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

I hate this spelling, TBH, but...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too, but it's consistent...

@lnicola lnicola mentioned this pull request Apr 24, 2023
2 tasks
@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

bors r+

bors bot added a commit that referenced this pull request Apr 24, 2023
389: Add SpatialRef::to_projjson() r=lnicola a=ttencate

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



390: Work around clippy lint bug r=lnicola a=lnicola

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Unblocks #389.

Co-authored-by: Thomas ten Cate <ttencate@gmail.com>
Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>
@bors
Copy link
Contributor

bors bot commented Apr 24, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request Apr 24, 2023
389: Add SpatialRef::to_projjson() r=lnicola a=ttencate

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Thomas ten Cate <ttencate@gmail.com>
@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

bors r-

@bors
Copy link
Contributor

bors bot commented Apr 24, 2023

Canceled.

bors bot added a commit that referenced this pull request Apr 24, 2023
390: Work around clippy lint bug r=lnicola a=lnicola

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

Unblocks #389.

Co-authored-by: Laurențiu Nicola <lnicola@dend.ro>
@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

Looks like we're still testing GDAL 3.0.4 on CI, and OSRExportToPROJJSON was added in 3.1.

@ttencate ttencate force-pushed the feature/srs_to_projjson branch from c39baff to 4ed76c4 Compare April 24, 2023 12:43
@ttencate
Copy link
Contributor Author

Looks like we're still testing GDAL 3.0.4 on CI, and OSRExportToPROJJSON was added in 3.1.

Thanks. Added a check.

@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

Please drop that PAM file.

bors d+

@bors
Copy link
Contributor

bors bot commented Apr 24, 2023

✌️ ttencate can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@ttencate ttencate force-pushed the feature/srs_to_projjson branch from 4ed76c4 to ba0f283 Compare April 24, 2023 12:46
@ttencate
Copy link
Contributor Author

ttencate commented Apr 24, 2023

Please drop that PAM file.

Sorry, yeah, running cargo test keeps creating that for me (Arch Linux, GDAL 3.6.3). Should it be gitignored?

@ttencate
Copy link
Contributor Author

bors r+

Thanks for the quick turnaround!

bors bot added a commit that referenced this pull request Apr 24, 2023
389: Add SpatialRef::to_projjson() r=ttencate a=ttencate

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Thomas ten Cate <ttencate@gmail.com>
#[cfg(any(major_ge_4, all(major_ge_3, minor_ge_1)))]
pub fn to_projjson(&self) -> Result<String> {
let mut c_projjsonstr = ptr::null_mut();
let options = ptr::null();
Copy link
Member

Choose a reason for hiding this comment

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

In the long run, I'd like us to support options where applicable:

https://gdal.org/doxygen/classOGRSpatialReference.html#a1a4b65b551d3a5ea91fb7fd8d9e1e4e8

@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

Sorry, yeah, running cargo test keeps creating that for me (Arch Linux, GDAL 3.6.3). Should it be gitignored?

I don't know, but I agree it's annoying :-).

@ttencate ttencate force-pushed the feature/srs_to_projjson branch from ba0f283 to 308d288 Compare April 24, 2023 12:50
@bors
Copy link
Contributor

bors bot commented Apr 24, 2023

Canceled.

@ttencate ttencate force-pushed the feature/srs_to_projjson branch from 308d288 to 5099745 Compare April 24, 2023 12:54
@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

tinymarble.tif.aux.xml strikes again!

@ttencate
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Apr 24, 2023
389: Add SpatialRef::to_projjson() r=ttencate a=ttencate

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Thomas ten Cate <ttencate@gmail.com>
@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

bors r-

@bors
Copy link
Contributor

bors bot commented Apr 24, 2023

Canceled.

@lnicola
Copy link
Member

lnicola commented Apr 24, 2023

You've added the PAM file again when changing the assert message.

@ttencate ttencate force-pushed the feature/srs_to_projjson branch from 5099745 to dcb09ae Compare April 24, 2023 18:28
@ttencate
Copy link
Contributor Author

Just added it to .gitignore so it won't bother anyone again.

@ttencate
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 24, 2023

@bors bors bot merged commit faf48e3 into georust:master Apr 24, 2023
@lnicola
Copy link
Member

lnicola commented Apr 25, 2023

Well, I was planning to fix TempFixture or the test to clean up better after itself 🙂.

@ttencate
Copy link
Contributor Author

Well, I was planning to fix TempFixture or the test to clean up better after itself slightly_smiling_face.

That's great, but let's keep this thing from wasting everyone's time until you get round to it :)

@metasim metasim mentioned this pull request May 18, 2023
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.

2 participants