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

Use u32 for party & candidate numbers #985

Merged
merged 6 commits into from
Feb 11, 2025

Conversation

michielp1807
Copy link
Contributor

As mentioned in #982, currently there cannot be more than 255 parties in Abacus because a u8 is used to store the party number. In the OpenAPI specification the pg_number is listed as int32, likely because OpenAPI does not have smaller integer types by default. The same thing also holds for the candidate numbers.

While it is unlikely there will be more than 255 parties in the Netherlands any time soon, using u8's is in my opinion unnecessarily restrictive. The couple of bytes saved by using u8s will in many cases remain empty anyways due to memory alignment.

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.77%. Comparing base (87cb37c) to head (c15460b).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
backend/src/apportionment/mod.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #985   +/-   ##
=======================================
  Coverage   89.76%   89.77%           
=======================================
  Files         248      248           
  Lines       12987    12995    +8     
  Branches     1318     1318           
=======================================
+ Hits        11658    11666    +8     
  Misses       1235     1235           
  Partials       94       94           

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

Lionqueen94
Lionqueen94 previously approved these changes Feb 7, 2025
praseodym
praseodym previously approved these changes Feb 7, 2025
Copy link
Contributor

@praseodym praseodym left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm! What do you think about adding a type alias type PGNumber = u32; (or similarly named)?

@michielp1807 michielp1807 force-pushed the u32-party-and-candidate-numbers branch from 90ce077 to 42bfe84 Compare February 10, 2025 10:09
@michielp1807
Copy link
Contributor Author

Thanks, lgtm! What do you think about adding a type alias type PGNumber = u32; (or similarly named)?

Good suggestion! I've updated the PR to use type aliases PGNumber and CandidateNumber

Copy link
Contributor

@Lionqueen94 Lionqueen94 left a comment

Choose a reason for hiding this comment

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

Could you run npm run gen:openapi in the frontend folder to make sure the openapi.ts is also updated :)

@michielp1807
Copy link
Contributor Author

Could you run npm run gen:openapi in the frontend folder to make sure the openapi.ts is also updated :)

Thanks for the reminder! Turns out I was missing #[schema(value_type = u32)] in a few places, which caused a u32 type to be introduced in the OpenAPI specification. Now it correctly uses numbers, which means the TypeScript types can remain the same.

@praseodym praseodym added this pull request to the merge queue Feb 11, 2025
Merged via the queue into kiesraad:main with commit ce5b730 Feb 11, 2025
18 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants