microsoft / pyright

Static Type Checker for Python
Other
12.45k stars 1.34k forks source link

Command-line flags should take precedence over `pyrightconfig.json` #7330

Closed DanielRosenwasser closed 4 months ago

DanielRosenwasser commented 4 months ago

Given a configuration file like the following:

{
    "pythonVersion": "3.11",
    "include": [
        "**/*",
    ],
}

and a file like the following:

# foo.py

type Yadda = int
type Badda = int

I would expect running pyright and pyright --pythonversion 3.11 to issue an error (which they do), but for pyright --pythonversion 3.12 to make that error disappear. Instead, I get something like the following:

# should error ✅
$ npx pyright -p pyrightconfig.json 
/some/dir/foo.py
  /some/dir/foo.py:3:1 - error: Type alias statement requires Python 3.12 or newer
  /some/dir/foo.py:3:1 - error: Type alias statement requires Python 3.12 or newer
1 errors, 0 warnings, 0 informations 

# should not error ❌
$ npx pyright -p pyrightconfig.json --pythonversion 3.12
/some/dir/foo.py
  /some/dir/foo.py:3:1 - error: Type alias statement requires Python 3.12 or newer
  /some/dir/foo.py:3:1 - error: Type alias statement requires Python 3.12 or newer

Note that I'm just using type aliases for demonstrative purposes to easily trigger a difference between 3.11 and 3.12.

When I spoke to Jake about this, he mentioned it was the case that the config file takes precedence, and I could remove the entry to pythonVersion - but that seems highly undesirable since I want 3.11 for the editor experience.

I suspect that this means that many many CI tasks trying to test multiple Python versions in a matrix are unintentionally misconfigured.

erictraut commented 4 months ago

Pyright is working as intended here, so I don't consider this a bug.

If you specify a pythonVersion in your config file, it takes precedence. Most people do not specify a pythonVersion in their config file, but if you do, it should (and will) be honored.

Out of curiosity, why did you provide a pythonVersion in your config? It's typically preferable to have the pythonVersion either auto-calculated based on the selected Python environment or specified on the command line.

DanielRosenwasser commented 4 months ago

Out of curiosity, why did you provide a pythonVersion in your config? It's typically preferable to have the pythonVersion either auto-calculated based on the selected Python environment or specified on the command line.

I was setting up a Python project and you provided me an option to specify which Python version I was using - so why wouldn't I specify it? 😄

I guess the other answer would be that this possibly provides a better out-of-the-box experience if you've just cloned a repo and haven't set up your environment yet.

erictraut commented 4 months ago

How were you "setting up a Python project"? What tool asked you which version of Python you were using?

DanielRosenwasser commented 4 months ago

So I'm working on a Python library. The library targets Python versions 3.11 and up, so I want to make sure that

  1. Contributors to the library don't use any 3.12+ features
  2. Type-checking and tests pass across 3.11 and 3.12 at the least.

Going back to how I created the project, I knew vaguely that I could create either a pyrightconfig.json or add to my pyproject.toml file. I skimmed through the options list and figured that in order to get a consistent build and editing experience it would be a best practice to specify the pythonVersion option.

Note that overriding options from the command line is the behavior in TypeScript and I think most other tools I've used.

I guess from your perspective, nobody should be setting the flag at all - but if that's the case, does it make sense to have the option? Because for the users who do need to specify their pythonVersion, they'll have to bend over backwards to get things working in CI.

jakebailey commented 4 months ago

I honestly didn't realize this is how it worked; I'm probably going to add something to pyright-action to warn when this happens as it even bit me when I submitted a CI workflow for TypeChat's python implementation.

I think the way I expected it to work was a left to right ordering of precedence, where the config file is implicitly loaded first then overwritten (or, if other flags, first then --project overwrites them too).

jakebailey commented 4 months ago

The next person who filed an issue on pyright-action (for other reasons) accidentally hit this too: https://github.com/mhammond/pywin32/pull/2102/files#r1506359143

Avasam commented 4 months ago

Out of curiosity, why did you provide a pythonVersion in your config? It's typically preferable to have the pythonVersion either auto-calculated based on the selected Python environment or specified on the command line.

I was just linked to this from @jakebailey .

Because editors don't simultaneously check multiple python versions at once, and the older versions are the ones most likely to lead to issues due to trying to use newer features. If my project supports, let's say 3.8-3.12, then I want all editors (w/o having to configure them individually) AND default runs of the pyright CLI to show errors if I try to, let's say, use 3.10 union syntax, or access an ABCMeta .

It's also weird to me that a configuration file option would override a CLI argument imo. CLI are always the "last item in the chain" in that any argument you set there is very much explicit and intended, you'd expect it. to override everything else. This allows CIs to have "the last word on how a command is run", without having to start manipulating files.

This assumption is currently in quite a few projects.

erictraut commented 4 months ago

Reopening issue as an enhancement request.

erictraut commented 4 months ago

This is addressed in pyright 1.1.352, which I just published.

jakebailey commented 4 months ago

Thank you!