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

Docstring argument fixes #3709

Closed
wants to merge 4 commits into from
Closed

Conversation

akx
Copy link
Contributor

@akx akx commented Mar 30, 2023

Description

  • Some of the docstrings for arguments were out of date c.f. the actual arguments.
    • For arguments I couldn't find descriptions for, I used TODO: undocumented; that's easy enough for someone to grep and fix.

@akx akx marked this pull request as ready for review March 30, 2023 11:01
@abidlabs
Copy link
Member

Thanks @akx we can fix these docstring issues and merge this. Just FYI, _js is intentionally undocumented because it is intended for internal use at this point, and it's API may change, until we close #2479

@gradio-pr-bot
Copy link
Collaborator

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-3709-all-demos

@akx
Copy link
Contributor Author

akx commented Mar 31, 2023

Thanks @akx we can fix these docstring issues and merge this. Just FYI, _js is intentionally undocumented because it is intended for internal use at this point, and it's API may change, until we close #2479

The parameter is being used in the wild, which was exactly why I was confused about the lack of docstring...

Would you like me to remove that change from the PR, or maybe add "experimental, may change" or something to the doc?

@akx akx force-pushed the fix-missing-docstring branch from bee7f40 to b833f53 Compare March 31, 2023 06:38
@abidlabs
Copy link
Member

abidlabs commented Mar 31, 2023

The parameter is being used in the wild, which was exactly why I was confused about the lack of docstring...

Makes sense, I'll add some documentation which explicitly states that this is an experimental parameter.

Btw @akx can you allow maintainers to push to this PR? I'll fix the remaining docstrings

@freddyaboulton
Copy link
Collaborator

Hi @akx ! This looks good to me. Can you re-open the PR with permission for outside commits so that @abidlabs can fix the remaining docstrings? Thank you.

@akx
Copy link
Contributor Author

akx commented Apr 3, 2023

Btw @akx can you allow maintainers to push to this PR? I'll fix the remaining docstrings

Done. Sorry for the delay

@abidlabs
Copy link
Member

abidlabs commented Apr 3, 2023

Thanks @akx! Fixing the docstrings now

@abidlabs
Copy link
Member

abidlabs commented Apr 3, 2023

I'm running into some issues trying to push changes to this branch. Started a new PR with these changes (#3740)

@abidlabs abidlabs closed this Apr 3, 2023
@akx akx deleted the fix-missing-docstring branch June 27, 2023 10:42
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.

4 participants