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: serialization of dataframes with NaN values and columns starting with digits #486

Merged

Conversation

fdorssers
Copy link
Contributor

@fdorssers fdorssers commented Aug 8, 2022

Closes #485

Proposed Changes

Update the regex string that's supposed to remove spurious leading commas in case the first field is None/NaN by also checking whether the first character of a column is a digit.

Locally pytest tests wasn't able to complete successfully due to an error in InfluxDBClientAsyncTest.test_delete_api. But I'm assuming that's due to a setup issue.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • pytest tests completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@fdorssers fdorssers force-pushed the nan-error-field-starting-with-digit branch from 9ba3d6a to 521e53a Compare August 8, 2022 13:06
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2022

Codecov Report

Merging #486 (9ba3d6a) into master (3842a9e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9ba3d6a differs from pull request most recent head 521e53a. Consider uploading reports for the commit 521e53a to get more accurate results

@@           Coverage Diff           @@
##           master     #486   +/-   ##
=======================================
  Coverage   89.42%   89.42%           
=======================================
  Files          39       39           
  Lines        3347     3347           
=======================================
  Hits         2993     2993           
  Misses        354      354           
Impacted Files Coverage Δ
...fluxdb_client/client/write/dataframe_serializer.py 98.26% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thank you very much for your PR 👍

There is only one requirement about README.md before we are be able merge your PR:

@fdorssers fdorssers force-pushed the nan-error-field-starting-with-digit branch from b620f8c to d2dc490 Compare August 8, 2022 13:20
@fdorssers fdorssers force-pushed the nan-error-field-starting-with-digit branch from d2dc490 to ae4110d Compare August 8, 2022 13:20
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@fdorssers thanks again for the PR 👍

LGTM 🚀

@fdorssers
Copy link
Contributor Author

No problem! Been using Influx for a while now, glad to contribute something back! 🥳

Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you for the fix and a test!

@powersj powersj merged commit 7a24f63 into influxdata:master Aug 8, 2022
@bednar bednar added this to the 1.32.0 milestone Aug 8, 2022
@fdorssers fdorssers deleted the nan-error-field-starting-with-digit branch August 9, 2022 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants