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

Rename custom-typeshed-dir, target-version and current-directory CLI options #14930

Merged
merged 5 commits into from
Dec 13, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Dec 12, 2024

Summary

This PR renames the --custom-typeshed-dir, target-version, and --current-directory cli options to --typeshed, --python-version, and --project as discussed in the CLI proposal document.
I added aliases for --target-version (for Ruff compat) and --custom-typeshed-dir (for Alex)

Test Plan

Long help

An extremely fast Python type checker.

Usage: red_knot [OPTIONS] [COMMAND]

Commands:
  server  Start the language server
  help    Print this message or the help of the given subcommand(s)

Options:
      --project <PROJECT>
          Run the command within the given project directory.
          
          All `pyproject.toml` files will be discovered by walking up the directory tree from the project root, as will the project's virtual environment (`.venv`).
          
          Other command-line arguments (such as relative paths) will be resolved relative to the current working directory."#,

      --venv-path <PATH>
          Path to the virtual environment the project uses.
          
          If provided, red-knot will use the `site-packages` directory of this virtual environment to resolve type information for the project's third-party dependencies.

      --typeshed-path <PATH>
          Custom directory to use for stdlib typeshed stubs

      --extra-search-path <PATH>
          Additional path to use as a module-resolution source (can be passed multiple times)

      --python-version <VERSION>
          Python version to assume when resolving types
          
          [possible values: 3.7, 3.8, 3.9, 3.10, 3.11, 3.12, 3.13]

  -v, --verbose...
          Use verbose output (or `-vv` and `-vvv` for more verbose output)

  -W, --watch
          Run in watch mode by re-running whenever files change

  -h, --help
          Print help (see a summary with '-h')

  -V, --version
          Print version

Short help

An extremely fast Python type checker.

Usage: red_knot [OPTIONS] [COMMAND]

Commands:
  server  Start the language server
  help    Print this message or the help of the given subcommand(s)

Options:
      --project <PROJECT>         Run the command within the given project directory
      --venv-path <PATH>          Path to the virtual environment the project uses
      --typeshed-path <PATH>      Custom directory to use for stdlib typeshed stubs
      --extra-search-path <PATH>  Additional path to use as a module-resolution source (can be passed multiple times)
      --python-version <VERSION>  Python version to assume when resolving types [possible values: 3.7, 3.8, 3.9, 3.10, 3.11, 3.12, 3.13]
  -v, --verbose...                Use verbose output (or `-vv` and `-vvv` for more verbose output)
  -W, --watch                     Run in watch mode by re-running whenever files change
  -h, --help                      Print help (see more with '--help')
  -V, --version                   Print version

@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Dec 12, 2024
Copy link
Contributor

github-actions bot commented Dec 12, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I still sorta think target-version more clearly communicates that it should be the Python version your code is targeting, which could be very different to the Python version you're using for local development. But hopefully we can address this with excellent documentation.

crates/red_knot/src/python_version.rs Outdated Show resolved Hide resolved
crates/red_knot/src/main.rs Outdated Show resolved Hide resolved
crates/red_knot/src/main.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser force-pushed the micha/rename-some-cli-options branch from 9d05782 to aa63ffa Compare December 12, 2024 12:29
Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thanks. Looking forward to not having to type --current-directory anymore.

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

LGTM!

)]
/// Run the command within the given project directory.
///
/// All `pyproject.toml` files will be discovered by walking up the directory tree from the project root,
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this sentence a bit confusing, because I thought the "project root" is defined by where we find a pyproject.toml with the expected config? It might make more sense to phrase this as something like "The project root will be found relative to this directory" or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair, I copied this from uv 🤭

@carljm
Copy link
Contributor

carljm commented Dec 12, 2024

I still sorta think target-version more clearly communicates that it should be the Python version your code is targeting, which could be very different to the Python version you're using for local development.

I don't really understand what the potential confusion would be here. If you are telling a type checker your Python version, what other meaning could there be other than "please typecheck this code for this Python version"? I guess I think it's totally fine if in simple cases people conflate "Python version I'm using for local development" with "Python version this code is targeting" -- wanting to separate those is a slightly more advanced use case, and I just don't see any likelihood that anyone who needs to separate those two will be confused about what --python-version means.

It has the advantage over --target-version that it's clear about "version of what" -- it's not a red-knot version, for example.

@AlexWaygood
Copy link
Member

I obviously don't feel strongly, or I wouldn't have approved!

I don't really understand what the potential confusion would be here. If you are telling a type checker your Python version, what other meaning could there be other than "please typecheck this code for this Python version"?

I think the potential confusion is that users might not understand why we need this setting at all, given that we already have other settings with which users can tell us the path to the Python executable they're using. If we always used the local-development Python version, we could just derive this information from that setting. But the setting doesn't really mean that at all -- it means "Please specify the minimum Python version you expect this app/library to support (not necessarily the version you're running now), so that we can emit errors if you use features or syntax that are newer than that".

I guess I think it's totally fine if in simple cases people conflate "Python version I'm using for local development" with "Python version this code is targeting" -- wanting to separate those is a slightly more advanced use case, and I just don't see any likelihood that anyone who needs to separate those two will be confused about what --python-version means.

It doesn't seem that advanced to me. I think it's pretty common that people use a version of Python for local development that's pretty new, but they nonetheless want it to be possible for other users to run their code on older versions of Python as well.

It has the advantage over --target-version that it's clear about "version of what" -- it's not a red-knot version, for example.

I can't recall anybody ever being confused about this on the Ruff issue tracker (Ruff uses --target-version)

@carljm
Copy link
Contributor

carljm commented Dec 12, 2024

I don't feel strongly either! Just trying to understand what I'm missing about the arguments either way.

users might not understand why we need this setting at all, given that we already have other settings with which users can tell us the path to the Python executable they're using.

One answer to this is simply that we want to avoid running a Python executable as part of red-knot settings derivation, and there's no reliable way to tell the Python version of a Python executable without running it. If there were such a way, I would be in favor of automatically deriving the Python version from the executable we are pointed towards, if a version isn't explicitly given.

I can't recall anybody ever being confused about this on the Ruff issue tracker (Ruff uses --target-version)

Fair enough, that's certainly relevant data!

@MichaReiser
Copy link
Member Author

It doesn't seem that advanced to me. I think it's pretty common that people use a version of Python for local development that's pretty new, but they nonetheless want it to be possible for other users to run their code on older versions of Python as well.

I expect most users will use requires-python in their pyproject.toml. The main use case that I see for -python-version`, and why it's important as a CLI flag, is to check a library for newer Python versions than their minimal supported Python version.

I also think that the setting is clearer because it is in environment.python-version. The main motivation for renaming is to be consistent with uv.

However, I agree with you that we could also just derive the value from --venv-path (to be renamed to --python).

@MichaReiser MichaReiser enabled auto-merge (squash) December 13, 2024 08:17
@MichaReiser MichaReiser merged commit c1837e4 into main Dec 13, 2024
20 checks passed
@MichaReiser MichaReiser deleted the micha/rename-some-cli-options branch December 13, 2024 08:21
dcreager added a commit that referenced this pull request Dec 13, 2024
* main:
  [red-knot] Display definition range in trace logs (#14955)
  [red-knot] Move the `ClassBase` enum to its own submodule (#14957)
  [red-knot] mdtest: python version requirements (#14954)
  [airflow]: Import modules that has been moved to airflow providers (AIR303) (#14764)
  [red-knot] Support `typing.TYPE_CHECKING` (#14952)
  Add tracing support to mdtest (#14935)
  Re-enable the fuzzer job on PRs (#14953)
  [red-knot] Improve `match` mdtests (#14951)
  Rename `custom-typeshed-dir`, `target-version` and `current-directory` CLI options (#14930)
  [red-knot] Add narrowing for 'while' loops (#14947)
  [`ruff`]  Skip SQLModel base classes for `mutable-class-default` (`RUF012`) (#14949)
  [red-knot] Tests for 'while' loop boundness (#14944)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants