Skip to content

Conversation

@skshetry
Copy link
Contributor

The following would be an example of someone using a branched callback.

with callback.branched(path_1, path_2, {}) as child:
    pass

The kwargs feel unnecessary to me. So I am proposing to remove that. It can be added later if someone requests it.

Also see #1460 (comment).

@martindurant
Copy link
Member

Since it's brand new, I suppose it's OK, but I mildly thing =None or **kwrags is better.

After all this, we should probably update text at https://filesystem-spec.readthedocs.io/en/latest/features.html#callbacks for users and somewhere better describe the process to write new callbacks.

@skshetry
Copy link
Contributor Author

Since branched() will be used deep inside put/get, I can't think of a good way to pass kwargs to it. Neither can I think of a good reason to do so. So far, the only use case in branch was to add a child callback.

@skshetry
Copy link
Contributor Author

But on isolation, **kwargs may be more ergonomic and extensible.

@skshetry skshetry force-pushed the remove-branched-kwargs branch from 16cd3e6 to 2d816d3 Compare January 11, 2024 04:18
@skshetry skshetry changed the title callbacks: remove kwargs positional argument from branched callbacks: replace kwargs positional argument with arbitrary kwargs in branched Jan 11, 2024
Comment on lines +61 to +62
**kwargs:
Arbitrary keyword arguments
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, this could be used to influence kwargs passed to put_file/get_file. I am not sure what the use case for that was, except of adding callback kwarg.

So this is deviating with branch in that regard.

Copy link
Member

Choose a reason for hiding this comment

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

So you vote to remove and add back later if there is a use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a use case for mutating kwargs in branched(). So, I'd just go with **kwargs (i.e. this PR).

I don't have any strong opinion against kwargs=None as it is similar but less ergonomic. But it does provide a way of mutating kwargs (but I don't see a use case for it).

We could also drop the third argument entirely, but that is not very flexible for future changes (I know I started with that, but 🤷🏽).

@skshetry skshetry force-pushed the remove-branched-kwargs branch from 2d816d3 to 1906396 Compare January 11, 2024 04:22
@martindurant martindurant merged commit 259215f into fsspec:master Jan 15, 2024
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