Clean up/Normalize docstrings

[allison-casey]

eat your broccoli kids 🥦

[Kodiologist]

Can we add typechecking to GitHub Actions or pytest before committing type annotations?

[allison-casey]

We can, but it'd require fixing all the type errors in the repo and Hy's python code has extensive implied type shadowing, circular imports, type assumptions, and unsafe type checks that would require a massive refactor that is outside the scope of this pr. This pr only formalizes the type hints made in existing docstrings. It doesn't fix any of the unsafe uses of those types in the codebase. I'm all for such an overhaul eventually, but adding it to ci now would be premature imo

[Kodiologist]

If the existing type information is not actually adhered to, it should be removed rather than retained, much less formalized, right?

[Kodiologist]

Also, while I haven't tried any of these tools yet, I was under the impression you can use them selectively, to gradually add typing to a codebase. So we can type the ones that are easy and maybe leave the hard ones for later.

[allison-casey]

the problem with Hy is that even the inferred typing is wrong. So even if we were to strip out every type hint from the codebase it would still fail any kind of ci we would do.

...
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:159:17 - error: Cannot access member "brackets" for type "String"
    Member "brackets" is unknown (reportGeneralTypeIssues)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:160:52 - error: Cannot access member "brackets" for type "String"
    Member "brackets" is unknown (reportGeneralTypeIssues)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:218:47 - error: "cond" is possibly unbound (reportUnboundVariable)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:623:72 - error: Cannot access member "value" for type "stmt"
    Member "value" is unknown (reportGeneralTypeIssues)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:631:19 - error: "elt" is possibly unbound (reportUnboundVariable)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:631:33 - error: "key" is possibly unbound (reportUnboundVariable)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:631:53 - error: "key" is possibly unbound (reportUnboundVariable)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:696:33 - error: Cannot access member "value" for type "stmt"
    Member "value" is unknown (reportGeneralTypeIssues)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:715:40 - error: "key" is possibly unbound (reportUnboundVariable)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:715:56 - error: "elt" is possibly unbound (reportUnboundVariable)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:716:33 - error: "elt" is possibly unbound (reportUnboundVariable)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:889:69 - error: Cannot access member "value" for type "stmt"
    Member "value" is unknown (reportGeneralTypeIssues)
  /home/alliejo/dev/forked/hy/hy/core/result_macros.py:892:17 - error: "match_case" is not a known member of module (reportGeneralTypeIssues)
77 errors, 0 warnings, 0 infos
Completed in 2.357sec

almost all of these errors aren't from the types that were lifted in this pr.

Pyright can be added as a pre-commit (pre-commit is awesome btw, and i'd highly recommend we adopt it in some way), but their is a startup delay to pyright which i don't think is worth it as a pre-commit. I'd probably just add it as a github actions hook

[Kodiologist]

It looks like each of those errors comes with an error type. Perhaps you can configure the program to ignore the error types that we haven't defended against. (After all, we just want the program to verify the type annotations, not to do a lot of other miscellaneous code-quality checks.) That could make sense, so long as it's as easy as adding the type names to a configuration file; I would like to avoid having to litter the code with magic comments to suppress warnings, as is required by a lot of linters.

But, since this could be a lot of work and you're just trying to clean up docstrings, maybe just reformat the docstrings, and leave the type hints in them for now.

[refi64]

I believe mypy is a lot more amenable to not trying to type-check annotation-less code, so it might be worth using instead in CI, at least for the moment. (That being said, are you sure you're not using pyright in strict mode?)

[allison-casey]

Moved type hints into docstrings. Although all the types were responsible for a grand total of one of the errors.

That could make sense, so long as it's as easy as adding the type names to a configuration file

It's not. I'd be happy to open another PR after this to fix the non type issues pyright finds. Many of them should absolutely be fixed regardless of whether or not we add pyright to CI

I believe mypy is a lot more amenable to not trying to type-check annotation-less code

I've personally found mypy errors to be much less intuitive than pyright and that pyright has much better type inference. mypy still throws many errors with no types anyways.

that being said, are you sure you're not using pyright in strict mode?

Its not. putting it in to strict mode turns 76 errors into 3000+ lmao

[Kodiologist]

Ugh. I shouldn't be surprised. Linters seem to usually be a solution worse than the problem. Thanks for looking into it.

[allison-casey]

While i disagree pretty heavily with that sentiment, I understand not wanting to introduce this without CI. I'll work on another PR that addresses the refactor need in the code base so we can introduce better code quality testing from a clean slate.