vincentarelbundock / pycountrycode

GNU General Public License v3.0
3 stars 3 forks source link

Update formatting and tests to pass CI checks #8

Open frank113 opened 11 months ago

frank113 commented 11 months ago

Summary

As of the most recent commit CI formatting checks are failing. The linting failures prevent tests from being run. On my local fork test suites such as test_regex_internal fail due to missing the polars import. While this behavior is expected as a result of making both polars and pandas optional the tests must be updated in a manner that allows for such dependencies to be optional.

Work Needed

  1. Update package formatting to pass the linting CI steps
  2. Update all tests to pass when polars and pandas are not installed

Considerations

I can jump on the first to-do item within the next day and have time to devote to the second item later in the week.

vincentarelbundock commented 11 months ago

That sounds awesome. Right now, I think that we are checking with 2 or 3 different linters. I worry a bit that it's overkill and that it will just lead to annoying errors, without tangible benefits in terms of code clarity.

I've heard some very good things about the ruff linter recently, so that might be something to look at. But don't have a strong preference over code style, so I'm happy to defer to you.

frank113 commented 11 months ago

Right -- things such as isort exist solely to enforce consistency within import blocks. While I have seen this utilized in incredibly large packages it may be overkill in this case. ruff is admittedly new to me but worth a look as a potential fit for this project.

frank113 commented 11 months ago

I have been working on this card and can provide a more detailed breakdown of the specific changes:

  1. Migrate existing tests that use Polars, such as test_regex_internal to their own file, test_*_polars.py. The tests within this file will be annotated such that the test is skipped if Polars is not installed: WIP
  2. Update the CI / CD process to run the Polars and non-Polars tests: WIP
  3. Update formatting to pass CI checks: Done

Certain tests must duplicated and re-imagined to use the dictionary representation of codelist without Polars. In the interest of ensuring the CI checks are passing the re-imagining of certain tests will be captured in a future issue.

vincentarelbundock commented 11 months ago

That all sounds good.

At the same time, I'm a bit worried that you're going through a lot of trouble for this dependency thing. The main failure points are probably in the conversions themselves. As long as we test with lists and not series, Polars should never be called internally, which would be equivalent to running it without the dependency.

My hunch is that the priority should be converting the corner case regex tests from R to Python, because the different engines might actually generate some false positive/negatives in weird cases.

That said, all of what you have proposed so far sound like definite steps, and I'm happy to defer to you based on what you feel is more important and feel like working on. Thanks for taking an interest in the package, I really appreciate it!

vincentarelbundock commented 11 months ago

My hunch is that the priority should be converting the corner case regex tests from R to Python, because the different engines might actually generate some false positive/negatives in weird cases.

For the record, I opened a new issue for this: https://github.com/vincentarelbundock/pycountrycode/issues/10