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

[TIDY FIRST] Type BaseRunner class #10607

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Commits on Aug 23, 2024

  1. Add typing to BaseRunner.__init__

    The `BaseRunner` class is a widely inherited class, and it's untyped.
    Sometimes the wrong thing gets passed into the classes that inherit
    `BaseRunner`. The "subclasses" of `BaseRunner` generally further
    restrict some of the available typing, (f.x. the type of node that
    can be passed in). In order to properly type those subclasses, we
    first should type `BaseRunner`. This is the start of that.
    QMalcolm committed Aug 23, 2024
    Configuration menu
    Copy the full SHA
    a0c0291 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    d678d73 View commit details
    Browse the repository at this point in the history
  3. Add typing for BaseRunner.run_with_hooks

    As part of this I had to switch the typing of node from `GraphMemberNode`
    to `ManifestNode`. We needed to do this be because `GraphMemberNode` was
    _too_ wide. Specifically, `GraphMemberNode` includes the `Metric` node
    which doesn't inherit from `NodeInfoMixin` and thus does not have a
    `update_event_status` method. Unfortunately moving to `ManifestNode`
    _excludes_ some nodes from the allowed typing that we'll probably later
    on find we need to include. Maybe that means some nodes should be moved
    to the `ManifestNode` definition. We might find that it would be more
    appropriate however to add an `ExecutableNode` or `RunnableNode` protocol
    definition, and then use that for typing.
    QMalcolm committed Aug 23, 2024
    Configuration menu
    Copy the full SHA
    01219eb View commit details
    Browse the repository at this point in the history

Commits on Aug 26, 2024

  1. Configuration menu
    Copy the full SHA
    2092a70 View commit details
    Browse the repository at this point in the history
  2. Add type hinting to error_result, ephemeral_result, and `from_run…

    …_result` of `BaseRunner`
    
    There are two type errors in this commit. The type hinting of `RunResult.node` and the `node`
    argument of `_build_run_result` are _different_! I tried to fix this in a previous commit
    by going from `GraphMemberNode` too `ManifestNode` as `GraphMemberNode` was too broad. It seems
    `ManifestNode` is too narrow unfortunately. Checking the typing of `RunResult.node`, which I
    should have done sooner, it uses `ResultNode`, which makes sense.
    
    The second type error is that the type hinting of `RunResult.status` and the `status` arguement
    of `_build_run_result` are _different_. The correct typing we should be that of `RunResult.status`.
    Both of these typing issues will be resolved in the next commit.
    QMalcolm committed Aug 26, 2024
    Configuration menu
    Copy the full SHA
    7a80aa3 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    f4dd99b View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    15ef78b View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    b909090 View commit details
    Browse the repository at this point in the history
  6. Add typing to node property of ExecutionContext

    We did this because in `BaseRunner.compile_and_execute` we weren't
    getting type completion for accessing `ctx.node.node_info`. Said another
    way, we didn't have the type context in `compile_and_execute` to know
    if a `node_info` property existed on the `node` of the `ctx`. By improving
    type hinting for `ExecutionContext` we were able to resolve this.
    QMalcolm committed Aug 26, 2024
    Configuration menu
    Copy the full SHA
    8cc7124 View commit details
    Browse the repository at this point in the history
  7. Add type hinting to execute and run of BaseRunner

    Because `execute` didn't have it's return type specified, it appeared to
    `run` that there was no expected return value. In turn `compile_and_run`
    was saying that the `result` it would return would _always_ be `None`. However
    that is most assuredly _not_ the case, and typing `execute` solves this.
    QMalcolm committed Aug 26, 2024
    Configuration menu
    Copy the full SHA
    436e1e8 View commit details
    Browse the repository at this point in the history
  8. Add type hinting to exception handling methods of BaseRunner

    Specifically methods:
    - `_handle_catchable_exception`
    - `_handle_internal_exception`
    - `_handle_generic_exception`
    - `handle_exception`
    
    Additionally we are defining "catchable errors" twice now. Once as a union of
    types in `_handle_catchable_exception` and as a tuple of classes in `handle_exception`.
    We should look at refactoring this into a single definition sometime, as a change to
    one should 1-1 correspond to a change in the other.
    QMalcolm committed Aug 26, 2024
    Configuration menu
    Copy the full SHA
    486447e View commit details
    Browse the repository at this point in the history
  9. Configuration menu
    Copy the full SHA
    3bd2972 View commit details
    Browse the repository at this point in the history
  10. Add type hinting to before_execute and after_execute of BaseRunner

    These two methods are essentially just additional logging based for the
    runner. They should never return anything. Typing them as such helps
    guarantee that so people don't end up doing some weird things.
    QMalcolm committed Aug 26, 2024
    Configuration menu
    Copy the full SHA
    d274046 View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    c3512ed View commit details
    Browse the repository at this point in the history
  12. Add type hinting to on_skip and fix unhandled potential runtime error

    In adding type hinting to `on_skip`, and `do_skip`, a potential runtime
    error was uncovered wherein we were accessing `self.skip_cause.status` in
    `on_skip` when the `skip_cause` can be `None`. On that edge case, a
    runtime error would be raised given how we were attempting to access
    the `status` property. We now handle this edge case, and we only discovered
    it because of the additional typing.
    QMalcolm committed Aug 26, 2024
    Configuration menu
    Copy the full SHA
    008e90a View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    3b2481b View commit details
    Browse the repository at this point in the history

Commits on Aug 28, 2024

  1. Update .changes/unreleased/Under the Hood-20240826-141843.yaml

    Co-authored-by: Emily Rockman <[email protected]>
    QMalcolm and emmyoop committed Aug 28, 2024
    Configuration menu
    Copy the full SHA
    f500372 View commit details
    Browse the repository at this point in the history