Skip to content

Conversation

tobymao
Copy link
Owner

@tobymao tobymao commented Oct 11, 2025

No description provided.

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

What about the column.replace calls in L331 & L341?

@tobymao
Copy link
Owner Author

tobymao commented Oct 11, 2025

it didn't fail any tests

@georgesittas
Copy link
Collaborator

I suggest being conservative in this case; the refactor feels inconsistent otherwise and we won't be able to easily reason about what went wrong in the future, if it happens.

@tobymao
Copy link
Owner Author

tobymao commented Oct 13, 2025

i don't think it's necessary because the names don't change, i don't think we should just add code for no reason

@georgesittas
Copy link
Collaborator

My concern was about the references changing, not the names. Also, L341 replaces a column with a number, so its name does change in that case.

In any case, the reason for my suggestion was that we'd be more faithful to the previous implementation with minimal overhead, thus minimizing the risk & we don't need to reason about how other parts are affected. The benefit of this PR is that we don't always call scope.clear_cache(), but only if necessary– that'd still be true.

Feel free to disregard if you're good with the change as-is.

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