On Thu, 2023-03-16 at 10:58 -0700, Daniel Latypov wrote: > Yikes. > I either forgot or never knew about --strict, thanks for pointing it out. > > Looking into it, I don't think it's worth using for kunit.py, details below. Oh, no argument, whatever works for you. I don't really even remember why I/we settled on mypy vs. pytypes, I don't remember really doing any comparison. I do find the whole type-checking, if occasionally a bit painful, to be of benefit though, at least for larger code bases (and by larger I think I probably would say "more than a single file that you can keep in your head entirely" :-) > Hmm, the untyped stuff is not too important since we also use pytype, > which is a lot smarter in that it infers types where not specified. Hmm, mypy normally does a bunch of inference too, but yeah might be less smart here, or maybe --strict just insists on more precision. > The other two though warrant some attention > * attr-defined: we don't care about this. We have a directive to > squash this "error" for pytype. Ugh, do we need another one for > pytype? You mean mypy? I guess yes, you'd need "# type: ignore". But funnily, I think both tools will insist that you place the comment on the same line as the error, and that doesn't really work since you can only have one comment on that line :-) However, it looks like pytype would also accept "# type: ignore" according to the docs. > * assignment: not actually a real error, it points to our type > annotations being a bit sloppy Not sure what you meant by this, but maybe I just don't see it with my version of mypy. > tools/testing/kunit/kunit.py:459: error: Incompatible types in > assignment (expression has type "List[str]", variable has type > "TextIO") [assignment] > > So it doesn't like that we assign a list[str] or a IO[str] to `kunit_output`. > But we're passing it in as Iterable[str], which both work as. > > Even explicitly annotating it as an Iterable[str] doesn't make it happy [1]. > I'll ignore this, then. I think you got that annotation wrong, at least as far as mypy is concerned? > sys.stdin.reconfigure(errors='backslashreplace') #pytype: disable=attribute-error > - kunit_output = sys.stdin > + kunit_output = sys.stdin # type[Iterable[str]] Doing kunit_output: Iterable[str] = sys.stdin actually fixes it for me, and you don't even need the second one: > else: > with open(cli_args.file, 'r', errors='backslashreplace') as f: > - kunit_output = f.read().splitlines() > + kunit_output = f.read().splitlines() #type[Iterable[str]] > which I think is because it assigned the type as Iterable[str] after the first one, and then was happy since splitlines() is Iterable[str] after all. So maybe pytypes is smarter about finding the best common type. Anyway ... I don't really care so much. I just noticed this, and had it been any more complex I'd probably have gone and just ignored kunit from my project. Oh, another unrelated thing: it kind of bothered me that even with kunit_parser.py you automatically get some output on stdout, it felt like kunit_parser.py was or should be more of a library, but in the end I don't really care so much since I needed to write a JSON file for our internal system and could just suppress the stdout entirely :) johannes
