fix(with_columns): require exactly one of pass_dataframe_as / columns_to_pass / on_input#1654
Open
Hore01 wants to merge 1 commit into
Open
fix(with_columns): require exactly one of pass_dataframe_as / columns_to_pass / on_input#1654Hore01 wants to merge 1 commit into
Hore01 wants to merge 1 commit into
Conversation
…_to_pass / on_input Signed-off-by: Olajumoke Akinremi <106763970+Hore01@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
with_columns_base.__init__is meant to enforce mutual exclusivity betweenpass_dataframe_as,columns_to_pass, andon_input— the docstring on each of those parameters says so. The current check raises only when exactly two are set:So a caller who specifies all three silently passes the validation and crashes later with a confusing downstream error. A caller who specifies zero of them also silently passes.
is None)This PR replaces the boolean arithmetic with the intuitive form:
The error message opener is also tightened:
"only one of"→"exactly one of".Tests
Four new tests in
tests/function_modifiers/test_recursive.py, exercising the base class directly via a minimal inline subclass:ValueErrorValueError(regression of the case that was already caught)ValueError(new — was the primary bug)Validation lives in the shared base class so no parallel
h_polars/h_sparktests are necessary. Testing againstwith_columns_basedirectly (rather than e.g.h_pandas.with_columns) is necessary because some subclasses pre-reject certain arg combinations before delegating to the base.Behaviour change
Callers passing 0 or 3 of the mutually-exclusive args used to silently pass the decoration step and crash later with confusing errors deep in the DAG-build path. After this PR they get a clean
ValueErrorat decoration time. This is the only intentional behavioural change. The documented contract has always been mutual exclusivity, so no caller should be relying on the silent-pass behaviour — but flagging the change explicitly so reviewers aren't surprised.Signed-off-by: Olajumoke Akinremi 106763970+Hore01@users.noreply.github.com