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

[Suggestion] Change around the arguments for the on_x callbacks #3

Open
KSneijders opened this issue Mar 22, 2023 · 2 comments
Open
Labels
Enhancement New feature or request

Comments

@KSneijders
Copy link
Contributor

KSneijders commented Mar 22, 2023

The first argument for the callbacks is the retriever: Retriever.

From testing and looking at the examples, I can see that it's not used a lot at all.
I've found myself changing the function signatures (from examples) from the first line below to the second.

# From example:
def set_timestamp_repeat(retriever: Retriever, instance: FileHeader):
    ...

# To this (look at retriever parameter)
def set_timestamp_repeat(_, instance: FileHeader):
    ...

Adding them the other way around allows me to leave out the second argument (which would then be Retriever).

Thoughts?

@Divy1211 Divy1211 added the Enhancement New feature or request label Mar 23, 2023
@Divy1211
Copy link
Owner

Divy1211 commented Mar 23, 2023

This part of the API can definitely do with some improvement because when it was initially made, its usage was not clear - hence the retriever argument has kind of just stuck around. It probably is a good idea to flip the arg order (note: your signature will still need to have the _ as the second arg. The retriever arg does have use cases though, so removing it is not an option)

@KSneijders
Copy link
Contributor Author

KSneijders commented Mar 23, 2023

I was under the impression that, like JS, you can leave out trailing arguments... That's unfortunate. I've checked out some SO posts and things like *_ would be the way to go for this it seems.

So moving them around to reflect the frequency of use would be nice but isn't as useful as I had hoped. Either way, would still like to see the change! 🙂

@Divy1211 Divy1211 reopened this Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants