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

user defaults #2014

Merged
merged 12 commits into from
Sep 26, 2016
Merged

user defaults #2014

merged 12 commits into from
Sep 26, 2016

Conversation

debris
Copy link
Collaborator

@debris debris commented Aug 28, 2016

The goal of this pr is to have one place (file) in which we store user options from previous launches.

Right now there are two options stored in UserDefaults:

  • pruning
  • tracing

UserDefaults could be also used for other options, eg. fatdb in #1974

@debris debris added the A0-pleasereview 🤓 Pull request needs code review. label Aug 28, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 86.37% when pulling bf8bf17 on user_default into 2d883c4 on master.

/// Get user defaults path
pub fn user_defaults_path(&self) -> PathBuf {
let mut dir = Path::new(&self.db).to_path_buf();
dir.push("user_defaults");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the path should be network-dependent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

}
}

pub fn tracing_switch_to_bool(switch: Switch, user_defaults: &UserDefaults) -> Result<bool, String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be UserDefaults method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Test would be good too - logic here seems to be pretty fragile.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this method fits UserDefaults well. Tests added in latest commit

@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 Aug 29, 2016
@arkpar
Copy link
Collaborator

arkpar commented Aug 29, 2016

Why do we need such a file if there's a way to get all the information from the database?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 86.37% when pulling ac24cfd on user_default into 2d883c4 on master.

@debris
Copy link
Collaborator Author

debris commented Aug 29, 2016

Why do we need such a file if there's a way to get all the information from the database?

Currently different modules are responsible for reading / writing user settings to db. I wanted to gather all of them in one place. I used a file, mostly because it's easier to debug user settings there.

@rphmeier
Copy link
Contributor

rphmeier commented Aug 29, 2016

Should store dapps hosts, rpc modules, etc. as well (probably future work)

@tomusdrw
Copy link
Collaborator

IMHO options you listed should be stored in config files: #880 - they don't force you to re-sync and are not part of "state" of the app.

@rphmeier
Copy link
Contributor

Yeah, those are probably a better place. Although it might start to get confusing with multiple configuration files for different purposes, particularly once we start to add presets into the mix (including user-made presets)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 86.361% when pulling edb52c3 on user_default into 6da60af on master.

@debris debris added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Sep 1, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 85.364% when pulling 79c7dc9 on user_default into 25e6a4e on master.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0e788d6 on user_default into * on master*.

@arkpar
Copy link
Collaborator

arkpar commented Sep 5, 2016

I don't like the idea of having an additional config file for defaults. Looks like an unnecessary complication to me. Just another maintenance burden with no real benefits. It would be sufficient to just add a message on startup if tracedb is enabled for debugging, same way we already display message for pruning strategy.
@gavofyork please review this.

@debris
Copy link
Collaborator Author

debris commented Sep 5, 2016

I would not call it a config. It's just a place where we store defaults

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 84.667% when pulling 7bd504b on user_default into 4e466f0 on master.

@debris debris added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 10, 2016
@rphmeier rphmeier mentioned this pull request Sep 12, 2016
@rphmeier rphmeier added the M4-core ⛓ Core client code / Rust. label Sep 13, 2016
@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. A0-pleasereview 🤓 Pull request needs code review. labels Sep 26, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 85.187% when pulling 0675aa3 on user_default into abcdc81 on master.

@debris debris merged commit 56eb97a into master Sep 26, 2016
jacogr added a commit that referenced this pull request Sep 26, 2016
* 'master' of https://github.com/ethcore/parity:
  user defaults (#2014)
  Fixing jit feature compilation (#2310)
  Lenient bytes deserialization (#2036)
  Fixing tests
  saturating add
  Remove crufty code
  saturating not overflowing
  Peek transaction queue via RPC (#2270)
  Avoid penalizing legit transactions
  Penalize transactions with gas above gas limit
  Improving txqueue logs
jacogr added a commit that referenced this pull request Sep 26, 2016
* js:
  user defaults (#2014)
  Fixing jit feature compilation (#2310)
  Lenient bytes deserialization (#2036)
  Fixing tests
  saturating add
  Remove crufty code
  saturating not overflowing
  Peek transaction queue via RPC (#2270)
  Avoid penalizing legit transactions
  Penalize transactions with gas above gas limit
  Improving txqueue logs
@arkpar arkpar deleted the user_default branch October 3, 2016 15:35
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