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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

dgawlowsky
Copy link
Contributor

@dgawlowsky dgawlowsky commented Feb 3, 2024 •
edited by github-actions bot
Loading

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Feb 3, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #2214 will not alter performance

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

Summary

✅ 6 untouched benchmarks

@codecov Codecov
Copy link

codecov bot commented Feb 3, 2024 •
edited
Loading

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
feat: Handling of "inf" and similar NaN values #2211
Open
@edgarrmondragon edgarrmondragon self-assigned this Feb 4, 2024
edgarrmondragon
edgarrmondragon reviewed Feb 5, 2024
View reviewed changes
singer_sdk/helpers/_flattening.py
@@ -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 •
edited
Loading

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.
Reviewers

@edgarrmondragon edgarrmondragon edgarrmondragon left review comments

@kgpayne kgpayne Awaiting requested review from kgpayne

Assignees

@edgarrmondragon edgarrmondragon

Labels
Community-Contributed PR
Projects
None yet
Milestone
No milestone
2 participants
@dgawlowsky @edgarrmondragon