Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix env::home_dir() deprecation warning. #9281

Closed
wants to merge 1 commit into from

Conversation

c0gent
Copy link
Contributor

@c0gent c0gent commented Aug 5, 2018

  • Import the dirs crate in util/dir.
  • Replace uses of env::home_dir() with dirs::home_dir().
  • Reexport dirs::home_dir.
  • Continue to use std::env::home_dir on Android.

@parity-cla-bot
Copy link

It looks like @c0gent hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@c0gent
Copy link
Contributor Author

c0gent commented Aug 5, 2018

[clabot:check]

@parity-cla-bot
Copy link

It looks like @c0gent signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@tomaka
Copy link
Contributor

tomaka commented Aug 5, 2018

Thanks for the PR. However we reverted the exact same change some time ago because the dirs crate only supports Windows, Linux and Mac.

@tomaka tomaka added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Aug 5, 2018
@c0gent
Copy link
Contributor Author

c0gent commented Aug 5, 2018

My mistake. I didn't see that PR.

Should I add some cfg attributes to keep Android on std::env then?

@c0gent c0gent force-pushed the c0gent-home-dir branch 3 times, most recently from 2d66415 to cd0d5ff Compare August 5, 2018 08:25
@c0gent
Copy link
Contributor Author

c0gent commented Aug 5, 2018

Updated with exceptions for Android.

@tomaka
Copy link
Contributor

tomaka commented Aug 5, 2018

In my opinion the cost of making the code dirty is higher than seeing this small warning when you compile. I would personally prefer an #[allow(deprecated)] and an issue opened, compared to having platform-specific code.

For me the proper fix is to change the code so that the Parity binary introduces $HOME into the library part, but that's a much bigger change.

@c0gent
Copy link
Contributor Author

c0gent commented Aug 5, 2018

I see what you mean and I hear you 100% about the code dirtyness but because it will be such a large amount of work to make such a minor thing work spotlessly, and because the drawbacks of the quick fix are so few (other than it bothering us), I think it would be better to merge and leave an open issue until the time can be spent.

@c0gent
Copy link
Contributor Author

c0gent commented Aug 5, 2018

I can optionally just leave the deprecation warnings enabled on Android as reminders (they are suppressed currently).

@tomaka
Copy link
Contributor

tomaka commented Aug 5, 2018

My number one preference would be to leave the warning for now and fix it properly.
My number two preference would be to put a single line #![allow(deprecated)] in util/dir/src/lib.rs.

I don't think having platform-specific code is a good thing here.

@c0gent
Copy link
Contributor Author

c0gent commented Aug 5, 2018

I understand completely. I can add a #![allow(deprecated)] but I really don't think that a blanket allow is a good idea here and I don't think it solves much (considering we'd still need the allow when dir::home_dir is called -- which is only once in this case). If they must be used, I prefer the 'warts' of piecemeal allows in order to highlight precisely what's being suppressed and prevents anything else from accidentally going under the radar. (I know you know all of this -- by the way I'm a long time glium/vulkano user, I've long appreciated your work and contributions!)

I can close this PR for now. I won't have time to do a proper implementation right away.

* Import the `dirs` crate in `util/dir`.

* Replace uses of `env::home_dir()` with `dirs::home_dir()`.

* Reexport `dirs::home_dir`.

* Continue to use `std::env::home_dir` on Android.

* Bump `util/dir` to 0.1.2.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A1-onice 🌨 Pull request is reviewed well, but should not yet be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants