georust / proj

Rust bindings for the latest stable release of PROJ
https://docs.rs/proj
Apache License 2.0
137 stars 44 forks source link

Idiomatic renaming of entities from libproj #134

Closed michaelkirk closed 2 years ago

michaelkirk commented 2 years ago

Note this PR targets your PR #133 @urschrei, and is a follow-up to this comment specifically: https://github.com/georust/proj/pull/133#discussion_r896109068

This proposal is an admittedly baroque dance, but I think it has some benefits...

If we were to just do the Conventional re-naming for libproj entities commit, legacy users of the Info trait would see an error like:

   warning: unused import: `Info`
     --> src/main.rs:1:18
      |
    1 | use proj::{Proj, Info};
      |                  ^^^^
      |
      = note: `#[warn(unused_imports)]` on by default

    error[E0599]: no method named `network_enabled` found for struct `Proj` in the current scope
       --> src/main.rs:5:19
        |
    5   |     assert!(!proj.network_enabled());
        |                   ^^^^^^^^^^^^^^^ method not found in `Proj`
        |
       ::: /Users/mkirk/.cargo/git/checkouts/proj-89eb02e1b32feb8b/5516e1f/src/proj.rs:256:8
        |
    256 |     fn network_enabled(&self) -> bool {
        |        --------------- the method is available for `Proj` here
        |
        = help: items from traits can only be used if the trait is in scope
    help: the following trait is implemented but not in scope; perhaps add a `use` for it:
        |
    1   | use proj::proj::HasInfo;
        |

With the additional inclusion of 6992649ac58ecd63d8744b33999cdfb5ce9bd448 users will not error, and just see a warning:

 warning: unused import: `Info`
     --> src/main.rs:1:18
      |
    1 | use proj::{Proj, Info};
      |                  ^^^^
      |
      = note: `#[warn(unused_imports)]` on by default

It's still potentially confusing to remove a trait and replace it with a struct of the same name, but I don't think any builds should break and it leaves us with idiomatic naming.