Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/es voting stat plugin #1983
base: develop
Are you sure you want to change the base?
Feature/es voting stat plugin #1983
Changes from 8 commits
19cc469
f1329e6
b690a33
7c5c124
4336e00
07d0a81
43ef521
47861c9
2fe34b1
3fa5581
e00daae
c0f3def
8ff6922
3736c90
c53bf9a
b53b254
72587d3
f47e5ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, unsure if we want to add such a specific signal. @abitmore @jmjatlanta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance I don't like it. Is there any way to avoid this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need all the data to properly feed the plugin, so AFAIK there is no way to avoid this. Everything is performed inside the
vote_tally_helper
and nothing is given outside.What exactly do you don't like? Is the signal signature too specific?
To make it more generic I could switch the emission signature to emit an object with the data. This would make it extensible. It would be up to the signal receiver to use the data or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood correctly,
void(const account_object&, const account_object&, const uint64_t)
means we need to emit the signal once per account, which IMHO is too many. I think it's easier if we add thevoting_stake
data directly intoaccount_statistics_object
as a member variable, although this would need a bit more memory.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right. The signal is emitted once per account.
Inserting data in
account_statistics_object
would mean double tracking of the data. Also it would then insert always (or make a check whether the plugin is active, but this seems weird IMHO). But the plugin kinda looses its purpose then.What would be if I gathered the data first and then emit the signal with all the data? This would call the signal only once and wouldn't require more memory to be used. Though still would require a check whether the plugin is active or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this resolves #211.