Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

**Deprecation**: clib.Session.virtualfile_from_vectors now takes only one argument #3522

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

seisman
Copy link
Member

@seisman seisman commented Oct 16, 2024

This is a POC PR.

In the Session.virtualfile_in method, we must prepare for _data that will be passed to different virtualfile_from_* methods. Currently, _data must be an iterable (e.g., _data = (data, ) even for the simplest case) and when pass it to _virtualfile_from, we need to unpack it like:

_virtualfile_from(*data)

The complexity is because, while most _virtualfile_from_* methods only take a single argument, the _virtualfile_from_vectors takes multiple arguments. The _virtualfile_from_vectors is defined like _virtualfile_from_vectors(*vectors) and must be called like _virtualfile_from_vectors(x, y, z).

This PR changes its definition to _virtualfile_from_vectors(vectors), and now it should be called like _virtualfile_from_vectors((x, y, z)).

With this breaking change, the virtualfile_in function can be simplified slightly, which I think is more readable. Then the question is, is it worth a breaking change?

Edit: Actually it's possible to introduce the new syntax without breaking backward compatibility. This is done in f5b0e8e.

@seisman seisman added maintenance Boring but important stuff for the core devs needs review This PR has higher priority and needs review. labels Oct 16, 2024
@seisman seisman force-pushed the refactor/virtualfile_from_vectors branch from a73a4bc to f5b0e8e Compare October 16, 2024 13:32
@seisman seisman changed the title POC: **Breaking**: clib.Session.virtualfile_from_vectors now takes only one argument **Deprecation**: clib.Session.virtualfile_from_vectors now takes only one argument Oct 16, 2024
@seisman seisman added this to the 0.14.0 milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs needs review This PR has higher priority and needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant