Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type hints to src/microprobe/code folder #38

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

patrick-rivos
Copy link
Contributor

This series has a few changes to start integrating type hints into the repo.

Commit 1: Run the yapf command from utils/ensure_style.sh on the /code folder

Commit 2: Add basic type hints to files in the /code folder.
Notes:

  • I added return types to the abstract classes. I typically like to avoid explicitly naming return types, as I think it encourages people to lie about the possibility of a None return type. These types may need to change as I understand/annotate more of the code base.
  • I removed unused imports before adding types. This results in some interesting cases like here where we remove Instruction and add it back as a type.
  • I migrated some **kwargs patterns to regular arg: type = default patterns as **kwargs are very challenging to type hint. Example
    • This does cause some challenges here where we need to propagate the types/defaults out to the sub-classes so the init method gets somewhat verbose.
  • Migrated outdated class ClassName(object): and ClassName(six.with_metaclass(abc.ABCMeta, object)): to class ClassName: and class ClassName(abc.ABC):
    • NOTE I think this might break compatibility with Python2, but I haven't checked. If that's important please let me know.
  • Value/Variable annotations are extremely confusing to me at the moment, so I've left them un-annotated.
    • Examples: 1, 2, 3
  • Replaced depricated @abc.abstractproperty with @property @abc.abstractmethod Example

Commit 3: The facade pattern does not play nicely with python tooling :(
Thankfully an easy workaround is just defining the class methods as an alias on that class.

Commit 4: get_all_subclasses does not (and AFAIK cannot) give useful type hints. Because we tightly control the call-site I feel comfortable wrapping the call in a function to provide type information.

Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
…n tooling

Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
Signed-off-by: Patrick O'Neill <patrick@rivosinc.com>
@patrick-rivos patrick-rivos mentioned this pull request Jun 23, 2023
13 tasks
@patrick-rivos
Copy link
Contributor Author

Ping - if this sort of PR is too large, I can just add type hints to files as I make other edits which should be smaller and more review-able.
If there's anything I can do to make PRs easier to review please let me know. I'm not super familiar with the github flow - I'm more used to mailing lists :)

@rbertran rbertran merged commit d3bf67e into IBM:master Jul 6, 2023
@rbertran
Copy link
Collaborator

rbertran commented Jul 6, 2023

Sorry for the delay.... hectic days traveling, changing timezones... and I missed the first notification (the ping worked ;) )

I reviewed the changes and all looked great (some comments below). The CI tests passed, so I merged this PR. If some corner case arises in the future, we'll fix it.

Some comments:

  • PR size is OK. I typically organize the PR by functionality/new feature or by bug fix. In this case, since these changes are more related to readability/refactoring is perfectly OK to organize them by directory or subdirectory.
  • Any code related to support vintage Python2 can be removed. We do support Python2 anymore.
  • Changing **kwargs to *args make sense. I used **kwargs extensively for fast prototyping and because of my laziness... now that the code is more stable, it should be safe to move to *args and be able to provide the benefits of type checking.
  • All the other changes, related to improve the readability, refactoring and enabling new Python tooling are all welcome. The code-base started in early python 2.2 or 2.4 and it was never "ported" properly to 3.x series... I just did the minimum changes to keep the code working.

Thanks for your contribution!

@patrick-rivos patrick-rivos deleted the src-code-type-hints branch July 6, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants