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

feat: VoterApi::vote now returns an Option. #5589

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

MxmUrw
Copy link
Contributor

@MxmUrw MxmUrw commented Jan 27, 2025

Pull Request

Closes: PRO-1979

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.
  • I have written and tested required migrations.
  • I have updated documentation where appropriate.

Summary

The voter api can now signal that it doesn't have anything to vote for by returning Ok(None). Previously, in such cases we had to return Err(..), which lead to the RetrierClient scheduling an exponential backoff.

Base automatically changed from refactor/PRO-1980/consolidate-electoralsystem-and-runner-traits to main January 27, 2025 13:20
@MxmUrw MxmUrw force-pushed the feat/PRO-1979/make-return-value-of-voterapi-optional branch from 3a8402d to 3dda26b Compare January 27, 2025 15:27
@MxmUrw MxmUrw mentioned this pull request Jan 27, 2025
4 tasks
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 0% with 33 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (5db9729) to head (636d7d4).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
engine/src/witness/sol.rs 0% 27 Missing ⚠️
engine/src/elections/voter_api.rs 0% 4 Missing ⚠️
engine/src/elections.rs 0% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #5589    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        511     511            
  Lines      90565   90479    -86     
  Branches   90565   90479    -86     
======================================
- Hits       64429   64317   -112     
- Misses     23595   23619    +24     
- Partials    2541    2543     +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MxmUrw MxmUrw marked this pull request as ready for review January 28, 2025 08:46
@MxmUrw MxmUrw requested a review from kylezs as a code owner January 28, 2025 08:46
The voter api can now signal that it doesn't have anything to vote for
by returning `Ok(None)`. Previously, in such cases we had to return `Err(..)`,
which lead to the RetrierClient scheduling an exponential backoff.
@MxmUrw MxmUrw force-pushed the feat/PRO-1979/make-return-value-of-voterapi-optional branch from 3dda26b to afd2e97 Compare February 3, 2025 17:39
@@ -176,6 +176,9 @@ where

pending_submissions.insert(election_identifier, (partial_vote, vote));
},
Ok(None) => {
info!("Voting task for election '{:?}' returned 'None' (nothing to submit).", election_identifier);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I feel like debug would be more appropriate here otherwise we'll see a lot of spam.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
info!("Voting task for election '{:?}' returned 'None' (nothing to submit).", election_identifier);
debug!("Voting task for election '{:?}' returned 'None' (nothing to submit).", election_identifier);

Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

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

Agree re: the log level otherwise looks good 👍

@MxmUrw MxmUrw added this pull request to the merge queue Feb 4, 2025
Merged via the queue into main with commit 1ffa4a9 Feb 4, 2025
49 checks passed
@MxmUrw MxmUrw deleted the feat/PRO-1979/make-return-value-of-voterapi-optional branch February 4, 2025 14:49
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