Skip to content

Modernize Gov Methods#2326

Merged
UdjinM6 merged 12 commits into
dashpay:developfrom
PastaPastaPasta:gov-modernize
Oct 11, 2018
Merged

Modernize Gov Methods#2326
UdjinM6 merged 12 commits into
dashpay:developfrom
PastaPastaPasta:gov-modernize

Conversation

@PastaPastaPasta
Copy link
Copy Markdown
Member

Only need to look at 7566796, currently based on #2324, will be rebased once #2324 has been merged

@PastaPastaPasta PastaPastaPasta changed the title Modernize Gov Methods [WIP] Modernize Gov Methods Sep 27, 2018
Copy link
Copy Markdown

@InhumanPerfection InhumanPerfection left a comment

Choose a reason for hiding this comment

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

Some Copy & Pasta 😜 went wrong. See inline comments.

Comment thread src/governance-vote.cpp Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

vote_outcome_enum_t

Comment thread src/governance-vote.cpp Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

std::map<vote_signal_enum_t, std::string>
and name mapSignalString instead of mapSignalsString?

Comment thread src/governance-vote.cpp Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Keep capitalized all returned strings? -or- Make this lowercase as well?

Comment thread src/governance-vote.cpp Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

std::map<std::string, vote_outcome_enum_t>

Comment thread src/governance-vote.cpp Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not familiar with formatting, can it handle nOutcome as %s?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure ping: @UdjinM6 @nmarley

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wouldn't experiment with this and would simply use %d :)

@PastaPastaPasta
Copy link
Copy Markdown
Member Author

🙈 🙈 🙈

@PastaPastaPasta
Copy link
Copy Markdown
Member Author

Please re-review

@PastaPastaPasta PastaPastaPasta changed the title [WIP] Modernize Gov Methods Modernize Gov Methods Oct 7, 2018
@UdjinM6 UdjinM6 added this to the 12.4 milestone Oct 8, 2018
UdjinM6
UdjinM6 previously approved these changes Oct 8, 2018
Copy link
Copy Markdown

@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

Comment thread src/governance-vote.cpp Outdated

const auto& it = mapOutcomeString.find(nOutcome);
if (it == mapOutcomeString.end()) {
LogPrintf("CGovernanceVoting::%d -- ERROR: Unknown outcome %d\n", __func__, nOutcome);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

%s for __func__? Same lower.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice catch!

@UdjinM6 UdjinM6 dismissed their stale review October 8, 2018 22:07

there are few LogPrintfs to fix

@PastaPastaPasta
Copy link
Copy Markdown
Member Author

Please re-review

@UdjinM6
Copy link
Copy Markdown

UdjinM6 commented Oct 9, 2018

hmmm... bd8d7ae is doing too much, I expected smth like

LogPrintf("CGovernanceVoting::%s -- ERROR: Unknown outcome %d\n", __func__, nOutcome);
...
LogPrintf("CGovernanceVoting::%s -- ERROR: Unknown signal %d\n", __func__, nSignal);

@PastaPastaPasta
Copy link
Copy Markdown
Member Author

My bad 🙈

Copy link
Copy Markdown

@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 8deb8e9 into dashpay:develop Oct 11, 2018
@PastaPastaPasta PastaPastaPasta deleted the gov-modernize branch October 11, 2018 15:10
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request May 21, 2019
* governance.* formatting and copyright bump

* manual changes gov.cpp

* Modernize ConvertVoteOutcome, ConvertSignalToString and  ConvertOutcomeToString

* move breaks

* add braces instead of inlining

* Revert "Modernize ConvertVoteOutcome, ConvertSignalToString and  ConvertOutcomeToString"

This reverts commit 41bc3d3.

* Modernize ConvertVoteOutcome, ConvertSignalToString and  ConvertOutcomeToString

* fix bugs

* make "NONE" lowercase

* revert merge error

* @InhumanPerfection comments fix

* whoops
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.

3 participants