refactor: Prepare nullable column by default

[gab23r]

Motivation

towards #20

Changes

Changing to nullable=False by default would be a breaking change. For now, this set nullable to None and warn if nullable is not explicitly set.

Question, how to handle warning in tests, should I ignore them ? should I explicitly set nullable=True, nullable=False everywhere ?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (54cbc75) to head (16a01e1).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #31   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        39    +3     
  Lines         1788      1868   +80     
=========================================
+ Hits          1788      1868   +80     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[AndreasAlbertQC]

Question, how to handle warning in tests, should I ignore them ? should I explicitly set nullable=True, nullable=False everywhere ?

I think most of the tests would be fine with nullable=False, so adding the new value there now will mean removal again soon (and it's a lot of lines).

I would propose to add dataframely/_deprecation.py and do this:

import os
import warnings
from collections.abc import Callable
from functools import wraps

TRUTHY_VALUES = ["1", "true"]


def skip_if(env: str) -> Callable:
    """Decorator to skip warnings based on environment variable.

    If the environment variable is equivalent to any of TRUTHY_VALUES, the wrapped
    function is skipped.
    """
    def decorator(fun: Callable) -> Callable:
        @wraps(fun)
        def wrapper() -> None:
            should_skip = os.getenv(env, "").lower() in TRUTHY_VALUES
            if should_skip:
                return
            fun()
        return wrapper
    return decorator



@skip_if(env="DATAFRAMELY_IGNORE_NULLABLE_DEFAULT")
def warn_nullable_default_change() -> None:
     # the actual warning goes here

and then we can import warn_nullable_default_change for use in Column. I think this is a neat solution because:

1. It provides a pattern for future deprecations
2. We can just set the environment variable to True in tests
3. It provides the user with an easy way to turn off the warnings if they don't care

WDYT?

[gab23r]

I don't know, I have no strong feeling, but more i think about it more I think that we should go for the simplest solution: Ignoring the warning in the test from the ini_options.
I will go forward this this, but would be happy to revert if necessary.

[AndreasAlbertQC]

@gab23r I like my version better, but I'm biased :D Let's get a tie breaker from @delsner or @borchero

[delsner]

I'm in favor of @AndreasAlbertQC's suggestion as I think it's a nice pattern for future deprecations and it's easy to implement and control.

[AndreasAlbertQC]

hey @gab23r ! It took @delsner @borchero and me a little while to coordinate and agree on how we want to proceed. Sorry for the delay, it's out first time navigating this particular question in dataframely :)

Here's the plan we agreed on:

1. We commit to making the actual breaking change only in a major release. We documented this commitment in #35. The exact timing of the major release is not set yet, but I think it's realistic to expect this in a few months.
2. For the future warning in this PR, please implement the suggestion I made above for a warning function + environment-based skip. In #35, we also documented a new DATAFRAMELY_NO_FUTURE_WARNINGS environment variable that we'd like to use for this. The reasoning is that we want users to be able to disable the warning also at deployment time without having to write new code.
3. For the dataframely tests, please just disable the warning in this PR. There is no need to migrate the tests. Our reasoning is that while we generally never want to disable warnings, future warnings coming out of our own code are a special case because a) future warnings never mean something is broken right now, only might be in the future and b) our CI will naturally catch actual breakage once the breaking change comes in.

Let me know if you're still up to implement these changes, otherwise I'm of course happy to help. Thanks for your patience.

[gab23r]

This sounds really good to me! I will implement it!

[AndreasAlbertQC]

Thanks @gab23r , almost there, I only have one more suggestion on the test.

[AndreasAlbertQC]

Thanks @gab23r ! LGTM now. @borchero feel free to merge when you're happy

[borchero]

Thanks for your work here @gab23r! I took care of the two remaining small things myself so that we can finally merge this :)