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

Fix #6209 - introduce standalone dir crate #7383

Closed
wants to merge 5 commits into from

Conversation

nicolasochem
Copy link
Contributor

  • created the dir crate in util
  • moved code from ethstore/src/dir/paths.rs to dir crate
  • rename dir module in ethstore to accounts_dir to distinguish it from the dir crate
  • rebase and include code review changes

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

Nicolas Ochem added 3 commits December 26, 2017 00:54
* created the dir crate in util
* moved code from ethstore/src/dir/paths.rs to dir crate
* rename dir module in ethstore to accounts_dir to distinguish it
  from the dir crate
* changes after @tomusdrw on openethereum#6952
@nicolasochem
Copy link
Contributor Author

@tomusdrw please review

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good! Minor grumble regarding platform module.


mod platform {
use std::path::PathBuf;
#[cfg(target_os = "macos")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What I mean with platform module is:

// re-export
pub use platform::*;

#[cfg(target_os = "macos")]
mod platform {
  pub const CACHE_PATH: &'static str = "$BASE/cache";
  pub fn parity_base() -> PathBuf { ... }
}

#[cfg(windows)]
mod platform {
  pub const CACHE_PATH: &'static str = "$LOCAL/cache";
  pub fn parity_base() -> PathBuf { ... }
}

So that you don't need to repeat those #[cfg( statements for all methods, since it might be a bit confusing to understand what is exposed where.

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. labels Dec 27, 2017
@nicolasochem
Copy link
Contributor Author

@tomusdrw fixed, please check again

@@ -28,6 +28,8 @@ use bigint::hash::{H64, H256};
use journaldb::Algorithm;
use helpers::{replace_home, replace_home_and_local};
use app_dirs::{AppInfo, get_app_root, AppDataType};
// re-export platform-specific functions
use platform::*;

#[cfg(target_os = "macos")] const AUTHOR: &'static str = "Parity";
#[cfg(target_os = "macos")] const PRODUCT: &'static str = "io.parity.ethereum";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to grumble again, would you mind moving this constants to platform as well?

@nicolasochem
Copy link
Contributor Author

@tomusdrw please check again

@svyatonik svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 28, 2017
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 28, 2017
@tomusdrw
Copy link
Collaborator

Looks perfect! 👍

@debris debris closed this Dec 29, 2017
debris added a commit that referenced this pull request Dec 29, 2017
@5chdn 5chdn added this to the 1.9 milestone Jan 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants