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 #6952

Closed
wants to merge 1 commit into from

Conversation

nicolasochem
Copy link
Contributor

Done so far:

  • 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

Is there a need to mutualize some code in the newly created
util/dir/src/lib.rs ? If so, I would need some guidance as of which
functions are to be mutualized.

Done so far:

* 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

Is there a need to mutualize some code in the newly created
`util/dir/src/lib.rs` ? If so, I would need some guidance as of which
functions are to be mutualized.
@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added this to the 1.9 milestone Nov 2, 2017
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Nov 2, 2017
@@ -21,7 +21,8 @@ use std::collections::HashSet;

use dapps;
use dir::default_data_path;
use helpers::{parity_ipc_path, replace_home};
use helpers::parity_ipc_path;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you keep the imports sorted?

r.replace("/", &::std::path::MAIN_SEPARATOR.to_string())
}

pub fn replace_home_and_local(base: &str, local: &str, arg: &str) -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doc missing.

@@ -14,7 +14,12 @@
// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

use std::fs;
extern crate app_dirs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add #![warn(missing_docs)]

base
}

#[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.

Might be better to group the functions into a platform module:

#[cfg(target_os = "macos")]
mod platform {
  fn parity_base() {}
  fn geth_base() {}
}

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 30, 2017
@tomusdrw tomusdrw added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Dec 21, 2017
@tomusdrw
Copy link
Collaborator

@nicolasochem any progress?

nicolasochem pushed a commit to nicolasochem/parity that referenced this pull request Dec 26, 2017
* 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

replaced by #7383

nicolasochem pushed a commit to nicolasochem/parity that referenced this pull request Dec 26, 2017
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants