Conversation
|
@vincekurtz I realized when playing around with Here's my philosophy on this right now. Type hints are really useful, but are still just hints. I don't think we should strongly enforce typing (especially during CI), but I do think we should keep up a standard of typing things (and calling people out in review). I also think that locally, it's not a bad idea to run I put some comments on the useful examples that the stricter |
| from ambersim import ROOT | ||
|
|
||
|
|
||
| def _check_filepath(filepath: Union[str, Path]) -> str: |
There was a problem hiding this comment.
Example: pyright complains about _check_filepath because the leading underscore implies it's a private function of the current file, but it's unused within the file. Further, I import this elsewhere, so it's clearly not private.
| save_model_xml(filepath, output_name=output_name) | ||
| mj_model = _modify_robot_float_base(output_name) | ||
| Path.unlink(output_name) | ||
| Path(output_name).unlink() |
There was a problem hiding this comment.
Example: this is actually just a straight up error that I resolve in a sibling branch, but I have no idea how it passed CI for so long. It actually occurs in a few other places, too.
| "reportMissingImports": true, | ||
| "reportMissingTypeStubs": false, | ||
| "reportGeneralTypeIssues": false, | ||
| "reportGeneralTypeIssues": true, |
There was a problem hiding this comment.
reportGeneralTypeIssues: this is the main setting change that caused all these "errors" to pop up that I fixed.
| def _modify_robot_float_base(filepath: str) -> mj.MjModel: | ||
| """Modifies a robot to have a floating base if it doesn't already.""" | ||
| # loading current robot | ||
| assert str(filepath).split(".")[-1] == "xml" | ||
| robot = mjcf.from_file(filepath, model_dir="/".join(filepath.split("/")[:-1])) | ||
| robot = from_file(filepath, model_dir="/".join(filepath.split("/")[:-1])) |
There was a problem hiding this comment.
Example: I just copy/pasted this type hint from other function signatures, but pyright complains that objects of type Path don't have a split function. It turns out that I only ever pass in str args into this function, so I changed the type hint to reflect that.
1 participant
Add more type checks with
pyright. This is a first stab just to gut check how much of a pain it is to enforce this officially vs. just as a strong suggestion for documentation (hint: it was pretty annoying).