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

fix(taps): update _flatten_record function by adding allow_nan=True in json.dumps #2214

Closed
wants to merge 3 commits into from

Conversation

dgawlowsky
Copy link
Contributor

@dgawlowsky dgawlowsky commented Feb 3, 2024

Copy link

codspeed-hq bot commented Feb 3, 2024

CodSpeed Performance Report

Merging #2214 will not alter performance

Comparing dgawlowsky:patch-2 (7d635b3) with main (7c77366)

Summary

✅ 6 untouched benchmarks

Copy link

codecov bot commented Feb 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7c77366) 88.47% compared to head (7d635b3) 88.47%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2214   +/-   ##
=======================================
  Coverage   88.47%   88.47%           
=======================================
  Files          54       54           
  Lines        4720     4720           
  Branches      919      919           
=======================================
  Hits         4176     4176           
  Misses        383      383           
  Partials      161      161           

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

@dgawlowsky dgawlowsky changed the title Update _flatten_record function by adding allow_nan=True in json.dumps fix(taps): update _flatten_record function by adding allow_nan=True in json.dumps Feb 4, 2024
@edgarrmondragon edgarrmondragon linked an issue Feb 4, 2024 that may be closed by this pull request
@edgarrmondragon edgarrmondragon self-assigned this Feb 4, 2024
@@ -430,7 +430,7 @@ def _flatten_record(
items.append(
(
new_key,
json.dumps(v, use_decimal=True, default=str)
json.dumps(v, use_decimal=True, default=str, allow_nan=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wdyt about making this configurable with two initial NaN options (e.g. fail and allow)?

Copy link
Contributor Author

@dgawlowsky dgawlowsky Feb 5, 2024

Choose a reason for hiding this comment

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

I gave it a first try, please see move the conversation to #2222 and close this PR if possible.

Edit: I put a draft into the name of the PR as I am still refining the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edgarrmondragon can you take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@edgarrmondragon please close this PR and see the proposal in #2222

@dgawlowsky dgawlowsky changed the title fix(taps): update _flatten_record function by adding allow_nan=True in json.dumps Draft: fix(taps): update _flatten_record function by adding allow_nan=True in json.dumps Feb 5, 2024
@dgawlowsky dgawlowsky changed the title Draft: fix(taps): update _flatten_record function by adding allow_nan=True in json.dumps fix(taps): update _flatten_record function by adding allow_nan=True in json.dumps Feb 6, 2024
@edgarrmondragon
Copy link
Collaborator

Closed in favor of #2222

@meltano meltano locked as resolved and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants