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

Idiomatic renaming of entities from libproj #134

Merged
merged 2 commits into from
Jun 21, 2022

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Jun 16, 2022

Note this PR targets your PR #133 @urschrei, and is a follow-up to this comment specifically: #133 (comment)

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 6992649 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.

`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 urschrei merged commit 9ee393d into public_pjinfo Jun 21, 2022
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