Skip to content

rename vars in mnsync to make more sense#2308

Merged
UdjinM6 merged 2 commits into
dashpay:developfrom
nmarley:mnsync-var-rename
Sep 30, 2018
Merged

rename vars in mnsync to make more sense#2308
UdjinM6 merged 2 commits into
dashpay:developfrom
nmarley:mnsync-var-rename

Conversation

@nmarley
Copy link
Copy Markdown

@nmarley nmarley commented Sep 19, 2018

nRequestedMasternodeAssets -> nCurrentAsset
nRequestedMasternodeAttempt -> nTriedPeerCount

Comment thread src/masternode-sync.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.

On first sight, it sounds like "peers currently in progress" or something like that. Maybe nTriedPeerCount is better?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agree, nPeerCount is confusing imo. Smth like nTriedPeerCount or simply nTriedPeers would be better I think.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I probably don't understand the code well enough then, but nTriedPeers would seem misleading, right? As I understand it, it's the count of peers with which we are trying to sync a particular asset at the given moment, right? So "nTryingPeers" would seem more appropriate, but I have a feeling my understanding might be off.

@UdjinM6 Can you explain to me (since I seem to be confused) what exactly this variable is keeping count of? Is my understanding in the previous paragraph not correct?

Copy link
Copy Markdown

@UdjinM6 UdjinM6 Sep 22, 2018

Choose a reason for hiding this comment

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

As I understand it -tried as we already asked this many peers for this asset while trying is like we are (still) asking this many peers for this asset. The funny thing is that the former is true for mnb/mnw because we ask for the whole set at once and the later is true for gobject/gobjvotes because we ask for a partial info in many steps. So any option is good for me, I wouldn't argue one over the other too much :) (EDIT: but tried sounds a bit more general and fits better imo 😄 )

nRequestedMasternodeAssets -> nCurrentAsset
nRequestedMasternodeAttempt -> nPeerCount
@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 29, 2018

Rebased onto latest develop and renamed var to nTriedPeerCount per discussion, ready for (re-)review.

@UdjinM6 UdjinM6 added this to the 12.4 milestone Sep 29, 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.

utACK

Should probably update PR description ;)

@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 30, 2018

👍 Updated description

@UdjinM6 UdjinM6 merged commit a5aca04 into dashpay:develop Sep 30, 2018
@nmarley nmarley deleted the mnsync-var-rename branch October 12, 2018 23:07
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request May 20, 2019
* rename vars in mnsync to make more sense

nRequestedMasternodeAssets -> nCurrentAsset
nRequestedMasternodeAttempt -> nPeerCount

* rename var to nTriedPeerCount
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