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.

Sign up for GitHub

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

Jump to bottom

Don't take test files into account when looking at whether a package affects multiple packages #155

Open
orta opened this issue Jul 19, 2020 · 0 comments
Open

Don't take test files into account when looking at whether a package affects multiple packages #155

orta opened this issue Jul 19, 2020 · 0 comments

Comments

@orta
Copy link
Contributor

orta commented Jul 19, 2020

See DefinitelyTyped/DefinitelyTyped#46112

Could also do with changing the 3rd statement in the main message for multiple packages on the whole too
@elibarzilay elibarzilay mentioned this issue Oct 27, 2020
Open
@elibarzilay elibarzilay mentioned this issue Nov 3, 2020
Merged
elibarzilay added a commit that referenced this issue Nov 5, 2020
@elibarzilay
More info shuffling and refactoring
9aca510 
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@orta