Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backports v0.18: PR's 14530 and 14365 #4331

Merged
merged 2 commits into from Aug 13, 2021

Conversation

pravblockc
Copy link

Backports v0.18: PR's 14530 and 14365

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

@pravblockc
Copy link
Author

pls see https://github.com/UdjinM6/dash/commits/pr4331

@UdjinM6 Thank you for the proposed change list. I am little bit confused about changes you proposed for bitcoin#14365, My aim is to bring up Python dead code linter.
Are these changes includes fixing issues on running linter?

@UdjinM6
Copy link

UdjinM6 commented Aug 11, 2021

Yes, my changes enable dead code linter in CI and fix all the issues found by it.

@pravblockc
Copy link
Author

Yes, my changes enable dead code linter in CI and fix all the issues found by it.

Thank you, all changes were brought in now, please review

UdjinM6
UdjinM6 previously approved these changes Aug 11, 2021
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

@github-actions
Copy link

This pull request has conflicts, please rebase.

UdjinM6
UdjinM6 previously approved these changes Aug 12, 2021
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 added this to the 18 milestone Aug 12, 2021
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

minor fix needed

src/rpc/blockchain.cpp Show resolved Hide resolved
MarcoFalke added 2 commits August 12, 2021 14:42
fa483e1 rpc: Add RPCHelpMan for machine-generated help (MarcoFalke)
fa0d36f rpc: Include rpc/util.h where needed for RPCHelpMan (MarcoFalke)

Pull request description:

  This introduces a manager for the RPC help generation and demonstrates its use of it in some RPCs.

  It is the first non-exhaustive step toward bitcoin#14378 and I will create pull requests for the next steps after this one is merged.

Tree-SHA512: 86f68322443ff01cd964aaf0ebe186be63fbebe4c47676cf7a622cc2b5305fd176bd57badfd1bbf788a036812253eb0dead74ecc3b30664c3e0d9392b2248054
…Travis

c82190c tests: Add Python dead code linter (vulture) (practicalswift)
590a57f tests: Remove unused testing code (practicalswift)

Pull request description:

  Add Python dead code linter (`vulture`) to Travis.

  Rationale for allowing dead code only after explicit opt-in (via `--ignore-names`):
  * Less is more :-)
  * Unused code is by definition "untested"
  * Unused code can be an indication of bugs/logical errors. By making the contributor aware of newly introduced unused code it gives him/her an opportunity to investigate if the unused code they introduce is malignant or benign :-)
  * Unused code is hard to spot for humans and is thus often missed during manual review
  * [YAGNI](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it)

  Based on bitcoin#14312 to make linter job pass.

Tree-SHA512: 4c581df7c34986e226e4ade479e0d3c549daf38f4a4dc4564b25564d63e773a1830ba55d1289c771b1fa325483e8855b82b56e61859fe8e4b7dfa54034b093b6
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for merging via merge commit

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit 03bca03 into dashpay:develop Aug 13, 2021
{
{"changeAddress", RPCArg::Type::STR_HEX, true},
{"changePosition", RPCArg::Type::NUM, true},
{"change_type", RPCArg::Type::STR, true},
Copy link

Choose a reason for hiding this comment

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

I feel like this parameter has been added here by mistake and is not really used by Dash.
@kittywhiskers can you comment? If so, I'll remove it in my next PR as I deal with this code there.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, this looks like a mistake, please correct :) (preferably in it's own PR)

Copy link

Choose a reason for hiding this comment

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

Done - #4550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants