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

Cleanup: remove unused arguments from default functions #517

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rd4398
Copy link
Contributor

@rd4398 rd4398 commented Nov 26, 2024

Fixes: #483

Copy link
Collaborator

@shubhbapna shubhbapna left a comment

Choose a reason for hiding this comment

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

I just remembered that it will break our documentation since i think it depends on the default functions to generate the docs for the plugin. Is there a workaround for that?

@tiran
Copy link
Collaborator

tiran commented Nov 27, 2024

We not only use the arguments in our docs. They are also used in downstream to compare signatures between default hooks and the plugin hooks.

IMO there is nothing to fix here. It is totally okay to pass additional args to a function if that's part of the API.

@shubhbapna
Copy link
Collaborator

They are also used in downstream to compare signatures between default hooks and the plugin hooks.

I don't think we should enforce that anymore or at the very least change the logic of how we are enforcing that. In fromager we are using a special invoke function to ensure that a function only receives the arguments it needs.

So downstream we will need to ensure that the arguments we are passing in our downstream overrides are a subset of all the possible arguments that can be passed. We don't need strict check of signatures.

It is totally okay to pass additional args to a function if that's part of the API.

IMO it adds technical debt and can make it confusing for initial contributors

@tiran
Copy link
Collaborator

tiran commented Dec 2, 2024

In my humble opinion, our special invoke function is not a good design. It's not how typical Python APIs are designed. In a typical Python API, it is the responsibility of the callee to deal with unnecessary arguments, e.g. with **kwargs.

@dhellmann
Copy link
Member

In my humble opinion, our special invoke function is not a good design. It's not how typical Python APIs are designed. In a typical Python API, it is the responsibility of the callee to deal with unnecessary arguments, e.g. with **kwargs.

Yeah, it was a hack to try to avoid introducing breaking changes into the downstream repo when we bumped fromager. It's not an ideal solution, and we should fix that downstream code to use **kwds.

How will we record the formal API of each plugin and then document it if we remove unused arguments from the default functions? Let's figure that out, and then see what that tells us about next steps.

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.

remove unused arguments from default functions
4 participants