RFC: Adding a bunch more type annotations from pyrefly #50
Conversation
Pyrefly looks like a pretty neat way to check and enforce type annotations, and I also enabled strict checking with Pylance in VSFR ... err, VSCode. Pyrefly type checking could even be enabled as a github workflow if it's not too annoying. https://pyrefly.org/en/docs/installation/#add-pyrefly-to-ci
| @@ -11,7 +11,7 @@ def decode_VS_DATA_BUF( | |||
| next_seq = None | |||
| while br.size() >= 7: | |||
| seq, eid, gid, ts_offset = br.unpack('<BBBi') | |||
| dt = base_time + datetime.timedelta(milliseconds=ts_offset * 10) | |||
| dt: datetime.datetime = base_time + datetime.timedelta(milliseconds=ts_offset * 10) | |||
There was a problem hiding this comment.
Is it required by Pyrefly to annotate the types of all variables?
In my opinion, annotations like nvsfr: int = len(vsfr_ids) or target_date: str = r.unpack_string() make the code feel overloaded - these types are trivial for a reader to infer and should also be easy for a static analyzer to deduce.
I do like annotations such as ret: list[int] = [] and annotations in function arguments.
Even if these annotations are mandatory for Pyrefly, I’m open to including them in this repo if you find them useful.
There was a problem hiding this comment.
I think there are levels of checking and annotation you can tune; there were some variables that didn't appear to need type annoations. 🤷🏻
I agree that some things are trivial to infer - len(thing) -> int - and that function argument and return value annotations are useful. It seems like the philosophy of behind pyre and pyrefly is to annotate everything (handy when your IDE can complain when you stuff the wrong data type into a variable) since python isn't strictly typed, so requiring type annotations is a way to make you be more careful.
I think you can close this PR, or just keep it for discussion about type checking and annotations in general; I'll do a cleaner one to ensure that as many functions as possible have annotated arguments and return values, at least where it's not ugly.
There was a problem hiding this comment.
Pyrefly infers and displays types for local variables mostly as a development aid, they don't have to be added to the code unless it helps with clarity.
It's probably a good idea to add annotations for function params and returns though, and Pyrefly will help infer the latter.
There was a problem hiding this comment.
pyrefly also doesn't guess the annotation properly depending on how you import something (IMO).
import datetime
now = datetime.datetime.now()the inferred type of now is datetime, but then datetime.now doesn't exist. If was to say from datetime import datetime then now:datetime = datetime.now() would do what I want, but since I said import datetime I'd have expected pyrefly to infer now:datetime.datetime.
Pyrefly looks like a pretty neat way to check and enforce type annotations; I also enabled strict checking with Pylance in VSFR ... err, VSCode.
Pyrefly type checking could even be enabled as a github workflow if it's not too annoying. Of course, not everyone uses vscode, but there's a CLI version...
Thoughts? This PR should probably not be merged, I mostly did this to see how big the diff would be, and if it's something we even want to do.