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

Cli providers params #1180

Merged
merged 8 commits into from
Nov 24, 2024
Merged

Cli providers params #1180

merged 8 commits into from
Nov 24, 2024

Conversation

getzze
Copy link
Collaborator

@getzze getzze commented Oct 17, 2024

closes #1179

This is a breaking change, for command line usage.

@etherealite
Copy link
Contributor

etherealite commented Oct 20, 2024

I like a lot of what I'm seeing here, getting rid of the click-option-group package is nice. Keeping all of the click usage confined to one module is clean.

I think I would prefer if this could be made a little bit closer to the approach that the Typer project uses. It combines using docstrings with an annotated function signature. See it in action in the CLI Options with Help section of their docs.

I would really really really prefer to not have anything syntactic in the docstrings. Using the docstring in the generated docs is great, but don't parse it.

class SomeProvider(Provider):
    def __init__(
            self,
            username: Annotated[str, Option(help="OpenSubtitles username")],
	    password: Annotated[str, Option(help="OpenSubtitles password")],
	)
        """
        Downloads subtitles from OpenSubtitles.
        
        [username] and [password] are required if not set in config.toml
        """
        ...
subliminal --help
Usage: subliminal [OPTIONS] COMMAND [ARGS]...

  Subtitles, faster than your thoughts.

Options:
  -c, --config FILE               Path to the TOML configuration file.  [default: C:\Users\etherea
                                  lite\AppData\Local\subliminal\subliminal\subliminal.toml]
...

Commands:
  cache     Cache management.
  download  Download best subtitles.

Providers:
  opensubtitles
  ---------------
  Downloads subtitles from OpenSubtitles.
        
  [username] and [password] are required if not set in config.toml

Instead, use the signature annotations for anything where you'd need to parse a docstring and inline the docstring into the generated documentation.

I think it's worth raising that hard coding the parameters that an extension can use is pretty restrictive. Who knows what kind of requirements might be needed by some future extension. Is there not some way to allow more flexibility in this regard?

Perhaps allowing aping a narrow subset of click's api and exposing that to extensions would work?

@getzze
Copy link
Collaborator Author

getzze commented Oct 21, 2024

I like the usage of Annotated type hints, thanks, it's much better than parsing the docstring.

I think it's worth raising that hard coding the parameters that an extension can use is pretty restrictive. Who knows what kind of requirements might be needed by some future extension. Is there not some way to allow more flexibility in this regard?

It's not the case! It takes all the arguments of the __init__ method and transform them into click options. I hard-coded arguments to SKIP for the refiners but not the providers.

@getzze getzze force-pushed the cli-providers-params branch from 123c0f7 to 7020cf9 Compare October 22, 2024 21:46
@getzze getzze requested a review from ptrcnull October 22, 2024 22:07
@getzze getzze force-pushed the cli-providers-params branch from 901bcda to f1d612d Compare October 22, 2024 22:23
@etherealite
Copy link
Contributor

I'll try and incorporate this and the examples you gave in #1177 into my provider soon.

@getzze getzze force-pushed the cli-providers-params branch from f1d612d to 8c42d40 Compare November 4, 2024 23:34
@getzze getzze marked this pull request as ready for review November 4, 2024 23:34
@getzze
Copy link
Collaborator Author

getzze commented Nov 4, 2024

I decided not to use Annotated type hint and parse the docstring. If needed we can do it in another PR.

@getzze getzze force-pushed the cli-providers-params branch from 8c42d40 to 2455934 Compare November 5, 2024 00:29
@getzze getzze force-pushed the cli-providers-params branch from 2455934 to b2a9c3c Compare November 24, 2024 15:43
@getzze getzze merged commit 2bd5375 into Diaoul:main Nov 24, 2024
23 checks passed
@getzze getzze deleted the cli-providers-params branch November 24, 2024 16:17
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.

Pass arguments to providers through the CLI
2 participants