Skip to content

Conversation

@adrioui
Copy link
Contributor

@adrioui adrioui commented Jan 11, 2026

Description of changes

  • Remove dead code in visualize.py for output_type.__name__ fallback (was marked as TODO for removal)
  • Add comprehensive tests for promote_list and promote_tuple utility functions (resolves TODO in test file)

@github-actions github-actions bot added the tests Issues or PRs related to tests label Jan 11, 2026
"val",
[
pytest.param([1, 2, 3], id="list"),
pytest.param({"a": 1}, id="dict"),
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this entire test:

  • the dict case isn't being used
  • we don't want to guarantee identity stability only equality stability

Copy link
Contributor

@NickCrews NickCrews Jan 13, 2026

Choose a reason for hiding this comment

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

well, I guess we can add to the contract that identity is preserved.

but still, we don't need the paramaterization. just simplify to

def test_promote_list_identity():
    val = [1,2,3]
    assert promote_list(val) is val

pytest.param((1, 2, 3), id="tuple"),
],
)
def test_promote_tuple_identity(val):
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify to

def test_promote_tuple_identity():
    val = (1,2,3)
    assert promote_tuple(val) is val

return node.output_type.__name__
except (AttributeError, NotImplementedError):
return "\u2205" # empty set character
return "\u2205" # empty set character
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the impact of this change?

@adrioui
Copy link
Contributor Author

adrioui commented Jan 14, 2026

Thanks for the review feedback! I've addressed all comments:

Test Simplifications (commits c94cbc7)

Simplified both identity tests per your suggestions:

test_promote_list_identity - Removed parameterization and dead code (the dict case was never actually tested due to the if isinstance(val, list) guard):

def test_promote_list_identity():
    """Test that lists are returned as-is (same object)."""
    val = [1, 2, 3]
    assert promote_list(val) is val

test_promote_tuple_identity - Removed unnecessary single-item parameterization:

def test_promote_tuple_identity():
    """Test that tuples are returned as-is (same object)."""
    val = (1, 2, 3)
    assert promote_tuple(val) is val

Impact Explanation for visualize.py Dead Code Removal

The output_type property was removed from all ops.Node classes in July 2022 (commit d2a3919). Since then, accessing node.output_type always raises AttributeError, making this code path unreachable.

The removed code and the remaining code produce identical behavior - returning "∅" for nodes without dtype or schema. Currently, the only node reaching this fallback is ops.JoinLink.

The existing test test_custom_expr_with_not_implemented_type in ibis/expr/tests/test_visualize.py verifies this fallback behavior continues to work correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Issues or PRs related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants