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

Stop using rawState in exportDataEnvelope #1903

Merged
merged 3 commits into from Dec 12, 2023
Merged

Conversation

ifosli
Copy link
Contributor

@ifosli ifosli commented Dec 11, 2023

📲 What

This is a cleaner way to handle any future server changes; I just didn't understand decoders well enough to to this initially. The actual behavior (unknown strings default to .unknown and anything that can't be parsed as a string throws an error) remains the same.

✅ Acceptance criteria

  • Tests pass
  • Project builds & personal data can still be requested

@ifosli ifosli added this to the release-5.12.0 milestone Dec 11, 2023
@ifosli ifosli self-assigned this Dec 11, 2023
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5e5cae9) 83.73% compared to head (9c7c6aa) 83.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1903      +/-   ##
==========================================
+ Coverage   83.73%   83.75%   +0.01%     
==========================================
  Files        1228     1229       +1     
  Lines      111837   111842       +5     
  Branches    29748    29749       +1     
==========================================
+ Hits        93649    93670      +21     
+ Misses      17163    17147      -16     
  Partials     1025     1025              

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

Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a comment

Choose a reason for hiding this comment

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

This LGTM, but I'd like to see a really quickie test just for a valid and an invalid enum value.

I'm also slightly surprised that the default implementation of Decodable for an Enum doesn't already do this - what happened before you implemented this change?

@ifosli
Copy link
Contributor Author

ifosli commented Dec 11, 2023

This LGTM, but I'd like to see a really quickie test just for a valid and an invalid enum value.

I'm also slightly surprised that the default implementation of Decodable for an Enum doesn't already do this - what happened before you implemented this change?

If it's just decoded directly (and there's no enum that matches the string found), it throws an error, which means the entire export data envelope doesn't get parsed. (That was the original cause of one of my starter bugs and it caused the "request my data" button to never be clickable.)

I didn't create tests for this initially because this file doesn't have any tests, but good call! Added now.

@amy-at-kickstarter
Copy link
Contributor

This LGTM, but I'd like to see a really quickie test just for a valid and an invalid enum value.
I'm also slightly surprised that the default implementation of Decodable for an Enum doesn't already do this - what happened before you implemented this change?

If it's just decoded directly (and there's no enum that matches the string found), it throws an error, which means the entire export data envelope doesn't get parsed. (That was the original cause of one of my starter bugs and it caused the "request my data" button to never be clickable.)

I didn't create tests for this initially because this file doesn't have any tests, but good call! Added now.

Ah, gotcha, so this is replacing an unsafe/exploding init with a safe one. That seems fine for this use case!

Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a comment

Choose a reason for hiding this comment

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

LGTM

@ifosli ifosli merged commit d98e4fe into main Dec 12, 2023
7 checks passed
@ifosli ifosli deleted the exportDataEnvelopeCleanUp branch December 12, 2023 17:41
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.

None yet

2 participants