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

Testing WEBHOOK_PAYLOAD_EXAMPLE deserialization #1732

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Testing WEBHOOK_PAYLOAD_EXAMPLE deserialization #1732

merged 3 commits into from
Oct 13, 2023

Conversation

jamesbraza
Copy link
Contributor

When deserializing with pydantic:

  • v1: it didn't care about missing fields
  • v2: it does care about missing fields

This PR simply defaults some test-time fields so they are no longer missing. It solves one of the CI issues hit in #1727.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks for noticing! I am very glad we had unit tests for that 😄

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 13, 2023

The documentation is not available anymore as the PR was closed or merged.

@Wauplin Wauplin merged commit 9f343e0 into huggingface:main Oct 13, 2023
14 checks passed
@jamesbraza jamesbraza deleted the fixing-test-parse-payload branch October 13, 2023 14:52
@jamesbraza
Copy link
Contributor Author

Thanks for the review and merge @Wauplin !

One comment is, there were other Optional fields, but I wasn't sure if all Optional fields should be defaulted. I only defaulted the ones the example payload had missing, I felt that added the least amount of assumptions. You'll want to take a look, and see if other fields should have had defaults too

@Wauplin
Copy link
Contributor

Wauplin commented Oct 13, 2023

One comment is, there were other Optional fields, but I wasn't sure if all Optional fields should be defaulted. I only defaulted the ones the example payload had missing, I felt that added the least amount of assumptions. You'll want to take a look, and see if other fields should have had defaults too

Hard to tell what's best but thanks for letting me know @jamesbraza! I think it is best to make all optional fields default to None actually.

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