-
Notifications
You must be signed in to change notification settings - Fork 121
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
Add support for parsing partials and solo blocks #268
Conversation
# Conflicts: # src/chia_log/handlers/daily_stats/stats_manager.py
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.
Thanks @kanasite, overall looks pretty good!
Let's remove unused and commented out parts of the code.
I'd like to also see some test coverage on the new parsers and handlers.
Should be quite straightforward if you use the existing ones as a template.
I can't test this myself now, so it would be great to hear from other members of the community on that. Anyone already running this change?
src/chia_log/handlers/daily_stats/stat_accumulators/found_partial_stats.py
Outdated
Show resolved
Hide resolved
src/chia_log/handlers/daily_stats/stat_accumulators/found_partial_stats.py
Show resolved
Hide resolved
@kanasite please also read the guide for contributors so that you can make the CI tests pass. |
ah great, sorry I was adding these via vim and dint notice about the CI tests, will update it as soon as I can |
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.
Great work @kanasite !
Don't forget to also update the README.md so that the Daily Stats block is up to date with the new addition of "blocks found" and "partials submitted".
https://github.com/martomi/chiadog#supported-notifications
I made some quick changes for the linting and also the feedbacks. For the test coverage, I probably will take a little longer to add when I have some time to spare |
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.
Thanks for addressing the feedback @kanasite!
This one still needs attention: #268 (comment) Unless we revert the commit that I referenced there, it will still continue spamming Found Proof notifications.
Would be awesome If you get a chance to do the tests within the following few days. Worst case I will release without tests and they can be added in a follow-up PR.
Sorry I missed that out, I guess I will add the tests in a follow-up PR |
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.
Looks good now 💯
I'm really looking forward to a release with this PR in it. My Pushover client is overly chatty right now. Is there an ETA or a release schedule published? Anything I can do to help test? |
I'm hoping that enough people were on the dev branch and will take the lack of issues submitted as confirmation that the change is good to go. Release will be out in the next half hour! |
Hi! I bundled in the |
Thanks @guydavis - this feedback really helps! The release is now out! |
Disabling proof notification (due to frequent partial alert), partials submitted added to daily stats, added specific block farmed parsing