Make PjInfo struct public #133

urschrei · 2022-06-10T23:36:34Z

Fixes #131

michaelkirk · 2022-06-11T11:17:03Z

src/proj.rs

@@ -1018,6 +1018,8 @@ impl convert::TryFrom<(&str, &str)> for Proj {
    }
 }

+/// Info about the current PROJ definition
+#[derive(Copy, Clone, Debug)]
 struct PjInfo {


Suggested change

struct PjInfo {

pub struct PjInfo {

😉

Also, I don't think it can be Copy - only Clone - because of the String fields.

🤦 night-PR strikes again

src/proj.rs

Co-authored-by: Corey Farwell <[email protected]>

michaelkirk · 2022-06-13T20:31:10Z

src/proj.rs

+///
+/// [PROJ reference documentation](https://proj.org/development/reference/datatypes.html?highlight=has_inverse#c.PJ_PROJ_INFO)
+#[derive(Clone, Debug)]
+pub struct PjInfo {


Suggested change

pub struct PjInfo {

pub struct ProjInfo {

Since this was private, we can freely change the name. I think it's nice to match the upstream name, or at least idiomatically translate them.

This is completely subjective, so feel free to take it or leave it.

We already have a Projinfo: https://docs.rs/proj/latest/proj/struct.Projinfo.html

...which comes from PJ_INFO. Agreed that your naming more accurately maps to the underlying structures

ahah oh darn, good point @frewsxcv.

With perfect hindsight, maybe an idiomatic conversion would map PJ_INFO to proj::Info and PJ_PROJ_INFO to proj::ProjInfo

But, I'd understand if doing that whole swap is more work/confusion than it's worth.

Ah yes, and I've broken the build. Swapping will be a breaking change, won't it? (Not necessarily a reason not to do it…)

6f89e01 👀

Oh no. 😆

Well @urschrei, if you want to see this through - maybe:

- pub struct Projinfo { + pub struct Info { ... } + #[deprecated(message = "renamed to Info")] + pub type Projinfo = Info

Swapping will be a breaking change, won't it?

I don't think so? I think you'd just need to correspondingly rename the (here-to-fore private) usages of PjInfo.

We can't use Info because we have a public trait named Info: https://github.com/georust/proj/blob/main/src/proj.rs#L231

😭

Anyone got a good suggestion for an alternative name?

oh my gosh what a pjckle. I surrender. 🏳️

Ok wait, I think I have a proposal. One minute.

Proposal: #134

I think it puts us in a nice place, but it's got some downsides - I know you're no fan of macros, and it's potentially confusing to replace a trait with a struct of the same name, so no hard feelings if you'd prefer to just leave the names as they are.

Co-authored-by: Michael Kirk <[email protected]>

`use proj::Info` to have access to these methods. This is a nice convenience, but especially important because we want to have `use proj::Info` refer to the newly renamed `Info` struct, not the legacy `Info` trait. So now, instead of legacy people getting a confusing method for trying to use the legacy trait, they'll just see an "unused import" warning.

urschrei · 2022-06-21T11:58:29Z

I've merged #134, but even though there's now a proj::proj_info method which returns a proj::ProjInfo struct, it's not visible in docs, and neither is the struct definition. I feel like I'm losing my mind.

michaelkirk · 2022-06-21T21:09:09Z

even though there's now a proj::proj_info method which returns a proj::ProjInfo struct, it's not visible in docs, and neither is the struct definition.

Addressed in #136

I feel like I'm losing my mind.

Yes, this is pretty maddening. 😆 I know it was my suggestion, but upon reflection, even though it's an idiomatic translation, proj.info() is a pretty rough name. What would you think about renaming proj.info() to something like proj.lib_info()?

I think that would contrast better with proj.proj_info().

// get details about the libproj installation
proj.lib_info();

// get details about this specific transformation
proj.proj_info();

urschrei · 2022-06-21T21:23:15Z

Yeah I think that's much clearer! Do you want to leave #136 as is, and I'll merge it tomorrow then do the rename in here?

michaelkirk · 2022-06-21T21:49:48Z

SGTM!

make `fn proj_info` public

urschrei · 2022-06-22T10:47:30Z

OK, this is done!

CHANGES.md

michaelkirk · 2022-06-22T16:09:10Z

src/proj.rs

+        ///
+        /// # Safety
+        /// This method contains unsafe code.
+        pub fn lib_info(&self) -> Result<Info, ProjError> {


I guess this is a breaking change now, but I think it's the right thing to do now that there is another *info method. I also don't expect very many people are actually using it.

Probably worth a new release note though - something like:

BREAKING: Getting information about the version of libproj installed was renamed from proj.info() to proj.lib_info()

Co-authored-by: Michael Kirk <[email protected]>

urschrei · 2022-06-23T08:40:50Z

bors r+

bors · 2022-06-23T08:56:53Z

Build succeeded:

ci result

135: Bump proj-sys to use PROJ 9.0.1 r=urschrei a=urschrei This will be rebased against #133 when it merges – there are no changes to `proj`. This PR bumps `PROJ` to v9.0.1. It's a bugfix release. Full list of fixes is here: https://github.com/OSGeo/PROJ/releases/tag/9.0.1 Note that I haven't bumped the CI PROJ versions as there should be no compat issues (although we're about to find out): The EPSG version has also been updated so we may initially see some test errors in `proj` where the bundled binary is in use. Co-authored-by: Stephan Hügel <[email protected]>

michaelkirk · 2022-06-23T20:03:47Z

Good job getting this merged! Sorry for turning it into such an odyssey. 😬

urschrei · 2022-06-23T20:11:15Z

No, I'm very happy with where it ended up, so def worth it, I think!

michaelkirk reviewed Jun 11, 2022

View reviewed changes

Make PjInfo struct public

52ec1d5

urschrei force-pushed the public_pjinfo branch from 3d2f0ca to 52ec1d5 Compare June 11, 2022 16:15

frewsxcv approved these changes Jun 12, 2022

View reviewed changes

src/proj.rs Show resolved Hide resolved

Update src/proj.rs

dfe7ea2

Co-authored-by: Corey Farwell <[email protected]>

michaelkirk reviewed Jun 13, 2022

View reviewed changes

urschrei and others added 3 commits June 13, 2022 21:33

Update src/proj.rs

6f89e01

Co-authored-by: Michael Kirk <[email protected]>

Conventional re-naming for libproj entities

ac63a9e

michaelkirk mentioned this pull request Jun 16, 2022

Idiomatic renaming of entities from libproj #134

Merged

urschrei mentioned this pull request Jun 21, 2022

Bump proj-sys to use PROJ 9.0.1 #135

Merged

michaelkirk added 2 commits June 21, 2022 14:00

document and make public method to get proj_info

dbb9e5b

make ProjInfo fields pub

3003195

urschrei added 2 commits June 22, 2022 11:33

Merge pull request #136 from georust/mkirk/pub-proj-info

c68bf8c

make `fn proj_info` public

Rename info to lib_info

e29d450

Fix tests

5064aca

michaelkirk reviewed Jun 22, 2022

View reviewed changes

urschrei and others added 4 commits June 22, 2022 17:12

Update CHANGES.md

7c338d3

Co-authored-by: Michael Kirk <[email protected]>

Fix tests

cfdd933

Fix accidental proj-sys changes mod

c94118b

Update CHANGES again

1c277ed

bors bot merged commit 8fe1f0a into main Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make PjInfo struct public #133

Make PjInfo struct public #133

urschrei commented Jun 10, 2022

michaelkirk Jun 11, 2022

urschrei Jun 11, 2022

michaelkirk Jun 13, 2022

frewsxcv Jun 13, 2022

michaelkirk Jun 13, 2022 •

edited

Loading

urschrei Jun 13, 2022

michaelkirk Jun 13, 2022 •

edited

Loading

michaelkirk Jun 13, 2022

urschrei Jun 15, 2022

michaelkirk Jun 16, 2022

michaelkirk Jun 16, 2022

michaelkirk Jun 16, 2022

urschrei commented Jun 21, 2022

michaelkirk commented Jun 21, 2022 •

edited

Loading

urschrei commented Jun 21, 2022

michaelkirk commented Jun 21, 2022

urschrei commented Jun 22, 2022

michaelkirk Jun 22, 2022 •

edited

Loading

urschrei commented Jun 23, 2022

bors bot commented Jun 23, 2022

michaelkirk commented Jun 23, 2022

urschrei commented Jun 23, 2022

Make PjInfo struct public #133

Make PjInfo struct public #133

Conversation

urschrei commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urschrei commented Jun 21, 2022

michaelkirk commented Jun 21, 2022 • edited Loading

urschrei commented Jun 21, 2022

michaelkirk commented Jun 21, 2022

urschrei commented Jun 22, 2022

michaelkirk Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

urschrei commented Jun 23, 2022

bors bot commented Jun 23, 2022

michaelkirk commented Jun 23, 2022

urschrei commented Jun 23, 2022

michaelkirk Jun 13, 2022 •

edited

Loading

michaelkirk Jun 13, 2022 •

edited

Loading

michaelkirk commented Jun 21, 2022 •

edited

Loading

michaelkirk Jun 22, 2022 •

edited

Loading