Skip to content

Conversation

@ronnie-llamado
Copy link
Contributor

Issue

Fixes #440

Description

  • Explicitly defines __init__ for Manifest,InternationalizedText,ContactInformation, and Language
    • Fails if init=False is defined in dataclass

Testing

  1. Set environment to 3.9.7
  2. make test

@keithrfung keithrfung added the hacktoberfest Issues for the Hacktoberfest label Oct 7, 2021
@keithrfung
Copy link
Contributor

@ronnie-llamado I'm gonna take a deep dive and test this. May take a bit. This looks great. If this works, may ask to make a change to README and CI to remove the version restriction.

@ronnie-llamado
Copy link
Contributor Author

@keithrfung I'm looking forward to what you potentially find in the deep dive. I'm still not entirely sure why it works without passing init=False into the dataclass (and when it fails if you do pass it in).

Another uncertainity: why did it only take explicitly defining __init__ in for Manifest, InternationalizedText, ContactInformation, and Language? AnnotatedString matches the same pattern (only inherits from CryptoHashable), but did not throw any errors in testing.

As far as the other changes: No problem, I'll have the README/CI changes queued up.

@keithrfung keithrfung self-requested a review October 7, 2021 20:15
@keithrfung
Copy link
Contributor

keithrfung commented Oct 8, 2021

@ronnie-llamado It's a creative fix but I believe this works. Add the README and I will be waiting to approve. ✅

I believe AnnotatedString doesn't throw because of the ordering of the elements. AnnotatedString has only optional parameters where all the others have both.

I will make an issue to remove these in the future.

The README and any other documentation can now say 3.9 instead of a specific version on the changes.

Copy link
Contributor

@keithrfung keithrfung left a comment

Choose a reason for hiding this comment

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

@ronnie-llamado
Copy link
Contributor Author

I believe AnnotatedString doesn't throw because of the ordering of the elements. AnnotatedString has only optional parameters where all the others have both.

I had the same thought at one point, but ContactInformation also only has optional parameters.

It actually looks like AnnotatedString is not being instantiated in the testing, so no errors are being thrown. When generating ContactInformation, emails are passed in straight from hypothesis and no phone numbers are passed in. These two are expected to be of type AnnotatedString.

https://github.com/microsoft/electionguard-python/blob/f3e2f09503cab2214706240404ca70d5f7a925b0/src/electionguard_tools/strategies/election.py#L178-L185

@keithrfung keithrfung merged commit f061efa into Election-Tech-Initiative:main Oct 8, 2021
@keithrfung keithrfung added the hacktoberfest-accepted Pull Request will count towards Hacktoberfest label Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest Issues for the Hacktoberfest hacktoberfest-accepted Pull Request will count towards Hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐞 Python 3.9.7 causes tests to fail

2 participants