-
Notifications
You must be signed in to change notification settings - Fork 70
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): add argument for nan handling strategy to _flatten_record #2222
Open
dgawlowsky
wants to merge
14
commits into
meltano:main
Choose a base branch
from
dgawlowsky:handle-nans-in-flattening
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
ba4684f
fix(taps): add argument for nan handling strategy to _flatten_record
f2ac32b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 1ca59a4
add default value to nan_strategy
e167225
Merge branch 'handle-nans-in-flattening' of github.com:dgawlowsky/sdk…
a01f351
update docstrings
dgawlowsky 0c60cb9
Merge branch 'main' into handle-nans-in-flattening
edgarrmondragon 45092a2
Merge branch 'main' into handle-nans-in-flattening
edgarrmondragon 7392479
Merge branch 'main' into handle-nans-in-flattening
edgarrmondragon 704758b
future msgspec compatibility
dgawlowsky 9e38fea
Merge branch 'handle-nans-in-flattening' of github.com:dgawlowsky/sdk…
dgawlowsky 841b3dd
fix for precommit checks
dgawlowsky 023d8a8
fix typing
dgawlowsky 0e09c3e
Merge branch 'main' into handle-nans-in-flattening
edgarrmondragon bcc3c45
Merge branch 'main' into handle-nans-in-flattening
edgarrmondragon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to support msgspec for serialization in the near future (#1784), and it defaults to making the values null. Do you think there's a way to make these options future-compatible with msgspec?
https://jcristharif.com/msgspec/supported-types.html#float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you specify a little more details? I am not very familiar with
msgspec
lib. In simplejson which is currently used there isignore_nan
argument which could be used. I am thinking about adding one more nan_strategy "convert_null" which will setignore_nan
toTrue
. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgawlowsky that makes sense to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgarrmondragon
Added this! sorry for the delay!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dgawlowsky! I'll review this soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @edgarrmondragon! Can you share the status of the review? We are using meltano in one of our projects and will be deploying do production soon, hence it will be easier to pull library from official distribution. Thanks in advance!