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

Clean up deprecated options and add CHECK macro #9036

Merged
merged 9 commits into from
Jul 9, 2018
Merged

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Jul 3, 2018

We have added some deprecated options but forgot to include them in the deprecated mod. This makes users not be given any warnings if options are deprecated. This PR adds them back. In addition, options are organized by the version they were deprecated, this makes it possible to make a set of deprecation hard errors in the future.

A CHECK macro is also added in usage!. We had a hard coded check conditions in the macro before. Using the new CHECK syntax we can move it out.

Affects #9017. Deprecated dapps options will need to be added to the deprecated mod.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 3, 2018
@sorpaas sorpaas added this to the 1.12 milestone Jul 3, 2018
@5chdn
Copy link
Contributor

5chdn commented Jul 4, 2018

Thanks for that. I'll wait with #8963 for this.

Copy link
Contributor

@ascjones ascjones 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!

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM. Minor question regarding arguments related to the dapps server.

FLAG flag_jsonrpc_off: (bool) = false, or |_| None,
"--jsonrpc-off",
"Equivalent to --no-jsonrpc.",

FLAG flag_webapp: (bool) = false, or |_| None,
"-w, --webapp",
"Does nothing; dapps server is on by default now.",
"Does nothing; dapps server is removed.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does nothing; dapps server has been removed.


ARG arg_dapps_port: (Option<u16>) = None, or |c: &Config| c.dapps.as_ref()?.port.clone(),
"--dapps-port=[PORT]",
"Dapps server is merged with HTTP JSON-RPC server. Use --jsonrpc-port.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these arguments say instead that the dapps server has been removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah right. I checked, those arguments indeed does nothing.

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 9, 2018
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

👍

@andresilva
Copy link
Contributor

deprecated::tests::test_find_deprecated needs to be updated with dapps settings that were removed (instead of replaced).

@andresilva andresilva merged commit c63452e into master Jul 9, 2018
@5chdn 5chdn deleted the cli-macro branch July 9, 2018 19:53
dvdplm added a commit that referenced this pull request Jul 10, 2018
* master:
  Clean up deprecated options and add CHECK macro (#9036)
  Replace `std::env::home_dir` with `dirs::home_dir` (#9077)
  fix warning in secret-store test (#9074)
  SeedHashCompute remove needless `new` impl (#9063)
  remove trait bounds from several structs (#9055)
  docs: add changelog for 1.10.9 stable and 1.11.6 beta (#9069)
  Enable test in `miner/pool/test` (#9072)
5chdn added a commit that referenced this pull request Jul 10, 2018
@ascjones ascjones mentioned this pull request Jul 16, 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. P5-sometimesoon 🌲 Issue is worth doing soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants