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

Use Pyright in CI #242

Merged
merged 9 commits into from
Mar 20, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
},
// Set *default* container specific settings.json values on container create.
"settings": {
// Enable pylance's pyright integration (disabled by default):
"python.analysis.typeCheckingMode": "basic",
"python.defaultInterpreterPath": "/usr/local/bin/python",
"python.linting.enabled": true,
"python.linting.mypyEnabled": true,
Expand Down
6 changes: 6 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ repos:
language: system
types: [python]
exclude: setup.py|run_tests.py|docs/conf.py
- id: pyright
name: pyright
entry: pyright
language: node
types: [python]
exclude: setup.py|run_tests.py|docs/conf.py
Copy link
Owner Author

@jab jab Mar 10, 2022

Choose a reason for hiding this comment

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

More just noting for the record than to take away from more important discussion:

I tried following the docs at https://github.com/microsoft/pyright/blob/main/docs/ci-integration.md#running-pyright-as-a-pre-commit-hook but found:

  • I had to remove pass_filenames: false, or else node would just hang
  • I removed additional_dependencies: ['[email protected]'] because it would duplicate the pyright version from requirements/lint.txt, and the local environment gets set up with the requirements/lint.txt dependencies already


- repo: https://github.com/Lucas-C/pre-commit-hooks
rev: v1.1.10
Expand Down
5 changes: 3 additions & 2 deletions bidict/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,8 @@ def inverse(self) -> 'BidictBase[VT, KT]':
self._invweak: 't.Optional[weakref.ReferenceType[BidictBase[VT, KT]]]' = None
# Also store a weak reference back to `instance` on its inverse instance, so that
# the second `.inverse` access in `bi.inverse.inverse` hits the cached weakref.
inv._inv, inv._invweak = None, weakref.ref(self)
inv._inv = None
inv._invweak = weakref.ref(self)
Copy link
Owner Author

Choose a reason for hiding this comment

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

The code before this change is semantically equivalent to the code after this change. But before this change, pyright (but not mypy) flagged this line, as follows:

  /Users/jab/src/bidict/bidict/_base.py:187:23 - error: Cannot assign member "_invweak" for type "BidictBase[VT@BidictBase, KT@BidictBase]"
    Expression of type "ref" cannot be assigned to member "_invweak" of class "BidictBase[VT@BidictBase, KT@BidictBase]"
      Type "ref" cannot be assigned to type "ReferenceType[BidictBase[KT@BidictBase, VT@BidictBase]] | None"
        TypeVar "_T@ReferenceType" is invariant
          Type "Self@BidictBase[KT@BidictBase, VT@BidictBase]" cannot be assigned to type "BidictBase[KT@BidictBase, VT@BidictBase]"
        Type cannot be assigned to type "None" (reportGeneralTypeIssues)

Questions:

  1. Is this a bug in pyright? (This is with the latest version, 1.1.228.)
  2. Is there a better way to appease pyright here?

I'm willing to make this change just to appease pyright, but it has two minor drawbacks:

  • In general, x = 1; y = 2; z = 3 # ... is slower than x, y, z = 1, 2, 3
  • In cases like this, where the code is just carrying out exactly what the comment before it explains (and which is the inverse of the two (more verbosely-written) assignments that precede it), the single-assignment version that was previously written is slightly preferable, since it more quickly gets to the point that the comment just explained (and more clearly implements the comment in its entirety).

Choose a reason for hiding this comment

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

In general, x = 1; y = 2; z = 3 # ... is slower than x, y, z = 1, 2, 3

Are you sure? That's the opposite of what I'd expect, because with the first variant you shouldn't pay the cost of creating an intermediate tuple. I tried some quick benchmarks and also found that the separate assignments are faster. It's a pretty marginal difference, though, so I wouldn't make a decision based on performance unless it's highly performance critical code.

As for the pyright error, I suspect it's a limitation in pyright's bidirectional type inference.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Are you sure? That's the opposite of what I'd expect...

It's the opposite of what I'd expect too, but when I saw this as #11 in https://youtu.be/YjHsOrOOSuI?t=1216 and reproduced it, it stuck with me ever since. Interesting you're not reproducing it now! I wonder if this changed in more recent versions of Python.

I wouldn't make a decision based on performance unless it's highly performance critical code.

For sure. (When I said "minor", this is what I should have said.:)

Choose a reason for hiding this comment

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

It's the opposite of what I'd expect too, but when I saw this as #11 in https://youtu.be/YjHsOrOOSuI?t=1216 and reproduced it, it stuck with me ever since. Interesting you're not reproducing it now! I wonder if this changed in more recent versions of Python.

(Sorry for going off on this tangent, but now I'm interested :) )

I tried code like def f(): x1, x2 = 1, 2; return x1, x2 for different numbers of variables (all on 3.6). Unpacking is faster at n = 10, but slower at n = 3 and n = 100. I think the issue is that (1, 2) gets turned into a single constant by the compiler, and at n = 10 that apparently makes the whole thing cheaper than repeated assignment would be. But at n = 100 the compiler gives up on generating a single constant tuple and builds it at runtime instead, so repeated assignment is faster again. But in real code you're probably not just assigning a tuple of constant ints, so it's pretty unlikely that you'd run into a situation where it's faster in practice.

Copy link

@erictraut erictraut Mar 10, 2022

Choose a reason for hiding this comment

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

This is intended behavior in pyright, and it is related to bidirectional type inference. Pyright applies bidirectional type inference for assignments if the LHS expression has a declared type. The expression inv._invweak does have a declared type, but the expression inv._inv, inv._invweak does not. So bidirectional type inference is not applied when you use tuple packing/unpacking.

The other problem here is that you have declared self._invweak to have the type Optional[weakref.ReferenceType[BidictBase[VT, KT]]]', but really it should be declared as Optional[weakref.ReferenceType[Self]]`. If it were declared as such, then this problem wouldn't exist because the types on the LHS and RHS of the assignment would already match even without the use of bidirectional type inference.

The problem is that the Self type was added only recently (for Python 3.11). It is supported in typing_extensions and in pyright, but I don't think mypy has support for it yet.

I don't know why mypy accepts the assignment in this case without emitting an error. I don't think it technically should, but I guess it's a gray area.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks for the tip about the Self type, I hadn't seen that! I just tested it though and it doesn't currently work with generics. At least, if you try to do something like the following, you get an error about Self not supporting indexing:

class BidictBase(Mapping[KT, VT]):
    def inverse(self) -> Self[VT, KT]:
        ...

Without that, I'm not sure how useful Self would be here, even if bidict did only have to support Python 3.11+.

# In e.g. `bidict().inverse.inverse`, this design ensures that a strong reference
# back to the original instance is retained before its refcount drops to zero,
# avoiding an unintended potential deallocation.
Expand Down Expand Up @@ -486,7 +487,7 @@ def _init_from(self, other: MapOrIterItems[KT, VT]) -> None:
self._fwdm.update(other)
# If other is a bidict, use its existing backing inverse mapping, otherwise
# other could be a generator that's now exhausted, so invert self._fwdm on the fly.
inv = other.inverse if isinstance(other, BidictBase) else inverted(self._fwdm)
inv: MapOrIterItems[t.Any, t.Any] = other.inverse if isinstance(other, BidictBase) else inverted(self._fwdm)
jab marked this conversation as resolved.
Show resolved Hide resolved
self._invm.update(inv)

#: Used for the copy protocol.
Expand Down
2 changes: 1 addition & 1 deletion bidict/_bidict.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ...
@t.overload
def pop(self, __key: KT) -> VT: ...
@t.overload
def pop(self, __key: KT, __default: t.Union[VT, DT] = ...) -> t.Union[VT, DT]: ...
def pop(self, __key: KT, __default: t.Any = ...) -> t.Any: ...
Copy link
Owner Author

Choose a reason for hiding this comment

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

Without this change, pyright (but not mypy) flags this code with:

  /Users/jab/src/bidict/bidict/_bidict.py:109:9 - error: Overload 1 for "pop" overlaps overload 3 and returns an incompatible type (reportOverlappingOverload)
  /Users/jab/src/bidict/bidict/_bidict.py:115:9 - error: Overloaded function implementation is not consistent with signature of overload 3
    Function return type "VT@MutableBidict | DT@pop" is incompatible with type "VT@MutableBidict"
      Type "VT@MutableBidict | DT@pop" cannot be assigned to type "VT@MutableBidict" (reportGeneralTypeIssues)
  1. Is this a bug in pyright and/or mypy?
  2. Is there a better way to appease pyright here?

Choose a reason for hiding this comment

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

Your overload is a bit squirrelly here. I think pyright is correct in pointing out the incompatible overlap. I'm not sure why mypy doesn't flag it in this case.

If I correctly understand the intent of this overload, I think this would be a better solution:

    @t.overload
    def pop(self, __key: KT, __default: t.Literal[MISSING]) -> VT: ...
    @t.overload
    def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ...
    @t.overload
    def pop(self, __key: KT) -> VT: ...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, @erictraut, your proposed type hints indeed appease pyright, but now mypy is rejecting them:

bidict/_bidict.py: note: In member "pop" of class "MutableBidict":
bidict/_bidict.py:108:6: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
        @t.overload
         ^
bidict/_bidict.py:108:6: note:      Superclass:
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT) -> VT
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def [_T] pop(self, KT, Union[VT, _T] = ...) -> Union[VT, _T]
bidict/_bidict.py:108:6: note:      Subclass:
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT, Any) -> VT
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def [DT] pop(self, KT, DT) -> Union[VT, DT]
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT) -> VT
bidict/_bidict.py: note: In function "pop":
bidict/_bidict.py:109: error: Parameter 1 of Literal[...] is invalid  [misc]
        def pop(self, __key: KT, __default: 't.Literal[MISSING]') -> VT: ...
        ^
bidict/_bidict.py:109:41: error: Variable "bidict._typing.MISSING" is not valid as a type  [valid-type]
        def pop(self, __key: KT, __default: 't.Literal[MISSING]') -> VT: ...
                                            ^
bidict/_bidict.py:109:41: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases

Also, Literal requires Python 3.8+, and TypeAlias requires 3.10+. Bidict currently supports 3.7+, and it'd be nice to avoid taking a dependency on typing_extensions.

Any other suggestions?

Choose a reason for hiding this comment

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

You can change the first overload to accept __default: MissingT.

Copy link
Owner Author

@jab jab Mar 11, 2022

Choose a reason for hiding this comment

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

I meant to say, I tried that too, and again, it made pyright happy but made mypy start failing, as follows:

    @t.overload
    def pop(self, __key: KT) -> VT: ...
    @t.overload
    def pop(self, __key: KT, __default: MissingT) -> VT: ...
    @t.overload
    def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ...

    def pop(self, key: KT, default: ODT[DT] = MISSING) -> t.Union[VT, DT]:
        ...
bidict/_bidict.py: note: In member "pop" of class "MutableBidict":
bidict/_bidict.py:108:6: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
        @t.overload
         ^
bidict/_bidict.py:108:6: note:      Superclass:
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT) -> VT
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def [_T] pop(self, KT, Union[VT, _T] = ...) -> Union[VT, _T]
bidict/_bidict.py:108:6: note:      Subclass:
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT) -> VT
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT, MissingT) -> VT
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def [DT] pop(self, KT, DT) -> Union[VT, DT]

Does this indicate a bug somewhere between typeshed and/or pyright and/or mypy?

Choose a reason for hiding this comment

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

I think mypy is right there. The superclass allows you to pass an unrelated type as the second argument, but the subclass doesn't.

Pyright doesn't seem to handle overrides involving overloads correctly. I'll report a bug.

Copy link
Owner Author

Choose a reason for hiding this comment

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

On line 111 of the latest revision, the subclass’s overload does allow you to pass an unrelated type:

def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ...

No?

Choose a reason for hiding this comment

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

Do you still get the same mypy error from that? I was going off the error in your previous message.

Copy link
Owner Author

@jab jab Mar 13, 2022

Choose a reason for hiding this comment

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

Yes I do. Sorry that wasn’t clear. (You can see there is still a “type: ignore [override]” comment, which would cause mypy to fail if it were unused, but the mypy check in CI is passing for the latest revision.)

The latest type hints:

    @t.overload  # temporarily remove 'type: ignore' comment from this line
    def pop(self, __key: KT) -> VT: ...
    @t.overload
    def pop(self, __key: KT, __default: DT) -> t.Union[VT, DT]: ...

    def pop(self, key: KT, default: ODT[DT] = MISSING) -> t.Union[VT, DT]:
        ...

The mypy output (with the 'type: ignore' removed as per above):

mypy bidict/_bidict.py
bidict/_bidict.py: note: In member "pop" of class "MutableBidict":
bidict/_bidict.py:108:6: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
        @t.overload
         ^
bidict/_bidict.py:108:6: note:      Superclass:
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT) -> VT
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def [_T] pop(self, KT, Union[VT, _T] = ...) -> Union[VT, _T]
bidict/_bidict.py:108:6: note:      Subclass:
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def pop(self, KT) -> VT
bidict/_bidict.py:108:6: note:          @overload
bidict/_bidict.py:108:6: note:          def [DT] pop(self, KT, DT) -> Union[VT, DT]
Found 1 error in 1 file (checked 1 source file)

From mypy's output, the only difference between the superclass and subclass is that the superclass uses the typing module's private _T typevar, and my subclass is using the bidict._typing module's DT typevar. But that shouldn't change the meaning of these types, should it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Filed python/mypy#12390 so this bug can be tracked properly.


def pop(self, key: KT, default: ODT[DT] = MISSING) -> t.Union[VT, DT]:
"""*x.pop(k[, d]) → v*
Expand Down
4 changes: 3 additions & 1 deletion bidict/_iter.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@

from collections.abc import Mapping
from operator import itemgetter
import typing as t

from ._typing import KT, VT, IterItems, MapOrIterItems


def iteritems_mapping_or_iterable(arg: MapOrIterItems[KT, VT]) -> IterItems[KT, VT]:
"""Yield the items in *arg* based on whether it's a mapping."""
yield from arg.items() if isinstance(arg, Mapping) else arg
it = iter(arg.items() if isinstance(arg, Mapping) else arg)
return t.cast(IterItems[KT, VT], it)
jab marked this conversation as resolved.
Show resolved Hide resolved


def iteritems(__arg: MapOrIterItems[KT, VT], **kw: VT) -> IterItems[KT, VT]:
Expand Down
5 changes: 2 additions & 3 deletions bidict/_orderedbidict.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ def popitem(self, last: bool = True) -> t.Tuple[KT, VT]:
raise KeyError('OrderedBidict is empty')
node = getattr(self._sntl, 'prv' if last else 'nxt')
korv = self._node_by_korv.inverse[node]
if self._bykey:
return korv, self._pop(korv)
return self.inverse._pop(korv), korv
kv = (korv, self._pop(korv)) if self._bykey else (self.inverse._pop(korv), korv)
return t.cast(t.Tuple[KT, VT], kv)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Without this change, pyright (but not mypy) flags this code, as follows:

  /Users/jab/src/bidict/bidict/_orderedbidict.py:60:16 - error: Expression of type "tuple[VT@OrderedBidict, Any]" cannot be assigned to return type "Tuple[KT@OrderedBidict, VT@OrderedBidict]"
    Tuple entry 1 is incorrect type
      Type "VT@OrderedBidict" cannot be assigned to type "KT@OrderedBidict" (reportGeneralTypeIssues)
  1. Is this a bug in pyright and/or mypy?
  2. Is there a better way to appease pyright here? The "fix" I came up with here makes the code noisier and no safer; I think the code was more readable before.
    1. Again, I can't add a # type: ignore here without causing mypy to fail with error: Unused "type: ignore" comment, and wish I at least had the option of doing # pyright: ignore on a per-line basis instead.
    2. I think the "reportGeneralTypeIssues" pyright check that is flagging this code is so broad, and can catch too many actual issues, to disable it globally.

Choose a reason for hiding this comment

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

This is an intentional difference in the way mypy and pyright handle overloads when some of the arguments are type Any. Mypy assumes that the return type of the overloaded call is Any in this case. Pyright uses the first overload that matches. Mypy's behavior is somewhat justifiable given that it is focused entirely on type checking. Pyright is used as both a type checker and a language server, and users rely on types for completion suggestions. When the return type is Any, no completion suggestions are possible, and users get very angry. So we've opted to not special-case Any arguments for overloads. The specific resolution rules for overloads are not specified in PEP 484, so it's reasonable to expect that there might be small variations between type checkers here.

The root issue here is that the variable korv has type Any. Is there a way you could provide a more accurate type in this case?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting. Makes sense, thanks for explaining.

The root issue here is that the variable korv has type Any. Is there a way you could provide a more accurate type in this case?

I tried adding a cast to provide a more precise type:

        korv = self._node_by_korv.inverse[node]
        if self._bykey:
            return korv, self._pop(korv)
        korv = t.cast(VT, korv)
        return self.inverse._pop(korv), korv

But now pyright errors with:

  /Users/jab/src/bidict/bidict/_orderedbidict.py:61:34 - error: Argument of type "VT@OrderedBidict" cannot be assigned to parameter "key" of type "KT@OrderedBidict" in function "_pop"
    Type "VT@OrderedBidict" cannot be assigned to type "KT@OrderedBidict" (reportGeneralTypeIssues)
  /Users/jab/src/bidict/bidict/_orderedbidict.py:61:16 - error: Expression of type "tuple[VT@OrderedBidict, VT@OrderedBidict]" cannot be assigned to return type "Tuple[KT@OrderedBidict, VT@OrderedBidict]"
    Tuple entry 1 is incorrect type
      Type "VT@OrderedBidict" cannot be assigned to type "KT@OrderedBidict" (reportGeneralTypeIssues)

Here pyright seems confused. If I add

        reveal_type(self.inverse)
        reveal_type(self.inverse._pop)

right before the return self.inverse._pop(korv), korv, I get:

  /Users/jab/src/bidict/bidict/_orderedbidict.py:61:21 - information: Type of "self.inverse" is "OrderedBidict[VT@OrderedBidict, KT@OrderedBidict]"
  /Users/jab/src/bidict/bidict/_orderedbidict.py:62:21 - information: Type of "self.inverse._pop" is "(key: KT@OrderedBidict) -> VT@OrderedBidict"

Could this be a bug in pyright?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Using a pyright: ignore here for now. Will come back to this if it is not in fact a bug in pyright.


def move_to_end(self, key: KT, last: bool = True) -> None:
"""Move the item with the given key to the end if *last* is true, else to the beginning.
Expand Down
41 changes: 41 additions & 0 deletions pyrightconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"exclude": [
"docs",
"tests",
],
"reportConstantRedefinition": "error",
"reportDuplicateImport": "error",
"reportFunctionMemberAccess": "error",
"reportIncompatibleMethodOverride": "error",
"reportIncompatibleVariableOverride": "error",
"reportInvalidStringEscapeSequence": "error",
"reportInvalidStubStatement": "error",
"reportInvalidTypeVarUse": "error",
"reportMatchNotExhaustive": "error",
"reportMissingTypeArgument": "error",
"reportMissingTypeStubs": "error",
"reportOverlappingOverload": "error",
"reportPropertyTypeMismatch": "error",
"reportSelfClsParameterName": "error",
"reportUnknownLambdaType": "error",
"reportUnknownParameterType": "error",
"reportUnsupportedDunderAll": "error",
"reportUntypedBaseClass": "error",
"reportUntypedClassDecorator": "error",
"reportUntypedFunctionDecorator": "error",
"reportUntypedNamedTuple": "error",
"reportUnusedClass": "error",
"reportUnusedFunction": "error",
"reportUnusedImport": "error",
"reportUnusedVariable": "error",
"strictDictionaryInference": true,
"strictListInference": true,
"strictParameterNoneValue": true,

// Leave these off for now:
"reportPrivateUsage": "none",
"reportUnknownArgumentType": "none",
"reportUnknownMemberType": "none",
"reportUnknownVariableType": "none",
"reportUnusedCallResult": "none",
Copy link
Owner Author

@jab jab Mar 10, 2022

Choose a reason for hiding this comment

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

(In this PR, I'm much less concerned about the following, so if this stands to take attention away from any of the other, more important threads here, let's discuss this further after the other threads are resolved. With that said...)

I based this configuration on @erictraut's suggestion in microsoft/pyright#3143 (comment). I would have kept this in TOML, which I generally prefer over JSON (especially strict JSON, where e.g. comments and trailing commas are unsupported), but it looks like pyright supports JSON5 in pyrightconfig.json. But if you want a standalone config file, it looks like pyright currently only supports pyrightconfig.json, not e.g. pyrightconfig.toml. (When configuration is meant for a single tool, and not meant to be shared by multiple tools, I prefer it to go in a dedicated config file for that tool. Putting some tools' config in pyproject.toml, while having to leave other tools' configs in their own dedicated config files, since they don't support pyproject.toml (and in some cases never will), makes it harder for me to remember where to find the config for a given tool - "is this one of the tools whose config is in pyproject.toml, or in its own config file?" - or whether the tool is non-obviously being influenced by any of the other config sections in pyproject.toml.)

Also, I was hoping pyright offered a way to express this configuration more like:

{
  // Enable all strict checks (current, as well as any added in future)
  // without duplicating/exploding them here:
  "strict": true,
  // Selectively override the following checks which were enabled via strict=true,
  // since these checks are known to be unhelpful in this codebase:
  "reportFoo": "none",
 }

But couldn't get that to work. Is this not currently possible?

Choose a reason for hiding this comment

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

No, that's not currently possible. You can specify which diagnostic rules you want to enable by default for all files, and then you can specify "strict" on a per-file or per-directory basis. The "strict" overrides the less-than-strict settings, not the other way around. This is designed primarily for incremental typing where a type base starts with just a few rules enabled for the bulk of the code base but new files and modules are strictly typed.

One big benefit of json over toml is that json files can be described using a schema. The Pyright LSP ships with a schema to its own config file, and VS Code (or other smart editors) can then use that schema to validate the config file and offer edit-time completion suggestions and documentation. None of that is currently possible with ".toml" files.

Copy link
Owner Author

@jab jab Mar 11, 2022

Choose a reason for hiding this comment

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

Thanks for explaining. I do think it’d still be useful to be able to go in the other direction too: start with a global strict=true, and then globally turn off a select few checks as needed.

Re toml, good point. Now I’ll have to search for tomlschema to see if anyone’s working on that yet!

Copy link
Owner Author

Choose a reason for hiding this comment

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

I opened microsoft/pyright#3198 so this suggestion can be tracked properly.

}
6 changes: 5 additions & 1 deletion requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ markupsafe==2.1.0
mccabe==0.6.1
# via pylint
nodeenv==1.6.0
# via pre-commit
# via
# pre-commit
# pyright
packaging==21.3
# via
# build
Expand Down Expand Up @@ -113,6 +115,8 @@ pylint==2.12.2
# via -r lint.in
pyparsing==3.0.7
# via packaging
pyright==1.1.228
# via -r lint.in
pytest==7.0.1
# via
# -r lint.in
Expand Down
1 change: 1 addition & 0 deletions requirements/lint.in
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pre-commit
pyright
pylint
# pylint runs against tests/*.py which import pytest and hypothesis
pytest
Expand Down
6 changes: 5 additions & 1 deletion requirements/lint.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ lazy-object-proxy==1.7.1
mccabe==0.6.1
# via pylint
nodeenv==1.6.0
# via pre-commit
# via
# pre-commit
# pyright
packaging==21.3
# via pytest
platformdirs==2.5.1
Expand All @@ -46,6 +48,8 @@ pylint==2.12.2
# via -r lint.in
pyparsing==3.0.7
# via packaging
pyright==1.1.228
# via -r lint.in
pytest==7.0.1
# via -r lint.in
pyyaml==6.0
Expand Down