Add static type hints #293
Conversation
|
Before I rebase this (looks like it will be quite a big task unfortunately), @aswaterman can I get a nod that this would be acceptable so I don't waste my time? |
|
@Timmmm Yes, I agree static type hints are an improvement. Sorry the refactoring will be painful. I might gently suggest that you make the CI change to verify the static type hints as part of the same PR, so that we don't regress, but since you are the one doing the work, I'm not in a position to make any demands. |
|
Great, thanks!
Sounds like a good idea; I'll do that. |
Timmmm
force-pushed
the
user/timh/types
branch
from
7a6e01b to
efcd4ca
Compare
|
Ok I have rebased, added it to CI/precommit and updated the commit & MR messages. It is built on top of the commits from #306 and #304 since they made it easier and they are easy wins. This found an additional issue (the missing |
There was a problem hiding this comment.
LGTM, except for the one nit that this appears to incorporate the changes being discussed in #306. Can you either factor that out (which I know might or might not be a pain) or wait until that PR is resolved?
|
Also #304. They're in separate commits though so I was just going to wait until they were merged (assuming it doesn't take too long) and then rebase. |
@aswaterman I have made some changes and left some comments out there, if you agree, we can proceed accordingly. |
Previously this generate a `instr_dict.yaml` file, however there is no need to use YAML here. The only advantages of YAML over JSON are that it is (debatably) easier for humans to write, making it suitable as human-edited input files. This is a machine-generated output file so JSON is better. The main advantages are: 1. It removes the dependency on PyYAML, which is the only external dependency of this project. Python external dependencies are quite a pain - PyYAML is a particularly troublesome one - and having no dependencies is very convenient (no need for venv etc.). 2. It means consumers of the file don't need to depend on PyYAML. Note this could have been done in an 100% backwards compatible way by keeping the file name unchanged (since JSON is valid YAML), however I figured there probably aren't that many users of this file, and since the only thing they need to change is the filename it's probably better to minimise confusion by renaming it. It also makes it clear that it is guaranteed to be JSON.
This makes the code easier to understand and navigate, and also detected a few of bugs: 1. Missing brackets on e.upper. (Fixed) 2. Not strictly related to types, but a lot of the regexes were not raw strings and therefore contained invalid escape sequences. Python prints a warning about these in recent versions. (Fixed) 3. Expression in `process_pseudo_instructions()` that is always false. (Not fixed) 4. Missing definition of `log_and_exit()`. (Fixed) This is validated via pre-commit in CI.
Timmmm
force-pushed
the
user/timh/types
branch
from
efcd4ca to
284a5fa
Compare
This makes the code easier to understand and navigate, and also detected a few of bugs:
process_pseudo_instructions()that is always false. (Not fixed)log_and_exit(). (Fixed)This is validated via pre-commit in CI.