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

Issue 8/pass ci polars skip #11

Conversation

frank113
Copy link
Contributor

@frank113 frank113 commented Nov 2, 2023

Summary

This PR addresses issue #8 to ensure that the CI checks pass. This PR is a stop-gap solution to clean up the CI checks so that future failures can be caught expeditiously. Some of the polars functionality blurs the line mentioned in the a comment in the parent issue. For a future PR it is my belief the polars tests should be re-worked. This PR keeps the polars functionality to ensure that scenarios are tested even if in a non-optimal way.

Changes Made

  1. Update formatting of countrycode to ensure that the linting CI check passes.
  2. Updated the test workflow to add steps to install polars and run tests denoted as *_polars
  3. Update custom_strategies to reflect the dictionary representation of codelist
  4. Add empty_string_to_null helper function to account for the dictionary representation of codelist coercing empty string values (previously seen as None natively) to None
  5. Migrate prior custom_strategies to custom_strategies_polars. Based on comments on the issue thread itself I would like to sunset these as soon as possible but am leaving them in for this PR until we have conversion unit tests for the dictionary representation of codelist
  6. Move test_numeric to test_conversion_polars. When using the dictionary representation of codelist the iso3n column was the string representation of a number rather than a number. We can solidify this behavior when porting the tests to not use polars
  7. Skip all tests that need polars if polars is not installed. The skips occur at the module and file levels.

Considerations

  • I created this PR with the intent of getting the tests to pass as my first objective. We can migrate the conversion tests and sunset some polars functionality in later PRs.

@frank113 frank113 marked this pull request as ready for review November 2, 2023 03:22
@vincentarelbundock vincentarelbundock merged commit 68c10a3 into vincentarelbundock:main Nov 2, 2023
3 checks passed
@vincentarelbundock
Copy link
Owner

Looks great, thanks a lot @frank113 , I really appreciate this!

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.

2 participants