Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

More info shuffling and refactoring #215

Merged
merged 1 commit into from
Nov 5, 2020
Merged

More info shuffling and refactoring #215

merged 1 commit into from
Nov 5, 2020

Conversation

elibarzilay
Copy link
Contributor

  • The code for a dangerLevel of MultiplePackagesEdited was broken:
    it used noNulls over a list of booleans, which means that
    MultiplePackagesEdited corresponded with just having multiple
    packages (not taking into account tests or criticals). This changes
    the code to use just a simple count of packages for now.

    I tried to fix it, but that exposes a problem with dangerLevel in
    general: it is sometimes used as an indicator for a single package
    literally (eg, allow all owners to request a merge if it starts with
    "Scoped") and sometimes as a generic danger situation. This is likely
    my fault since pretty much any use of it confused me (since it's
    defined as a hard-to-remember combination of different things).

    For the same reason, I think that it'd be best to try to remove it and
    use the information directly. Some of the following changes are going
    in this direction. I will revisit the issues around it when this is
    done (eg Improve "multiple packages" label? #206 / Don't take test files into account when looking at whether a package affects multiple packages #155).

  • Use an extended editsInfra field instead of dangerLevel === "Infrastructure".

  • Working toward the above, move the getDangerLevel computation (and
    the dangerLevel field, and the DangerLevel type) to
    ExtendedPrInfo since there are no more uses of it in pr-info. The
    next step is to split it into fields that express the relevant parts
    instead of a single level value.

  • Move authorIsOwner from a PrInfo field to a computed
    ExtendedPrInfo one. It's meaning is also changed: previously, it
    would be on if the author is an owner of any package, and now it's
    on if the author is an owner of all packages (if more than one).
    This currently affects only the corresponding label. Two
    tests (45836, 45946) updated for this.

  • Aggregate all review info into a single getReviews (much revised
    from analyzeReviews) that returns a review: ReviewInfo[], holding
    all of the relevant information. Remove all of the other fields, and
    add the ones that are needed into the ExtendedPrInfo.

    There are some minor diffeences --- for example, previously all stale
    reviews were kept, and now it's only one review kind per person so at
    most one stale review. These differences only affect the derived
    information, but no test changes.

  • Note that the computation of approvalFlags moved to extendPrInfo
    (in compute-pr-actions), where it can be changed to adapt it to
    address People should not be able to self-merge until maintainers of all affected packages have approved a change. #178.

* The code for a `dangerLevel` of `MultiplePackagesEdited` was broken:
  it used `noNulls` over a list of booleans, which means that
  `MultiplePackagesEdited` corresponded with just having multiple
  packages (not taking into account tests or criticals).  This changes
  the code to use just a simple count of packages for now.

  I tried to fix it, but that exposes a problem with `dangerLevel` in
  general: it is sometimes used as an indicator for a single package
  literally (eg, allow all owners to request a merge if it starts with
  "Scoped") and sometimes as a generic danger situation.  This is likely
  my fault since pretty much any use of it confused me (since it's
  defined as a hard-to-remember combination of different things).

  For the same reason, I think that it'd be best to try to remove it and
  use the information directly.  Some of the following changes are going
  in this direction.  I will revisit the issues around it when this is
  done (eg #206 / #155).

* Use an extended `editsInfra` field instead of `dangerLevel ===
  "Infrastructure"`.

* Working toward the above, move the `getDangerLevel` computation (and
  the `dangerLevel` field, and the `DangerLevel` type) to
  `ExtendedPrInfo` since there are no more uses of it in `pr-info`.  The
  next step is to split it into fields that express the relevant parts
  instead of a single level value.

* Move `authorIsOwner` from a `PrInfo` field to a computed
  `ExtendedPrInfo` one.  It's meaning is also changed: previously, it
  would be on if the author is an owner of *any* package, and now it's
  on if the author is an owner of *all* packages (if more than one).
  This currently affects only the corresponding label.  Two
  tests (45836, 45946) updated for this.

* Aggregate all review info into a single `getReviews` (much revised
  from `analyzeReviews`) that returns a `review: ReviewInfo[]`, holding
  all of the relevant information.  Remove all of the other fields, and
  add the ones that are needed into the `ExtendedPrInfo`.

  There are some minor diffeences --- for example, previously all stale
  reviews were kept, and now it's only one review kind per person so at
  most one stale review.  These differences only affect the derived
  information, but no test changes.

* Note that the computation of `approvalFlags` moved to `extendPrInfo`
  (in `compute-pr-actions`), where it can be changed to adapt it to
  address #178.
Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, looks like net lines removed 👍🏻

@elibarzilay elibarzilay merged commit 9aca510 into DefinitelyTyped:master Nov 5, 2020
@elibarzilay elibarzilay deleted the more-shuffling branch November 5, 2020 14:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants