Improve "multiple packages" label? #206
Comments
|
Good question, one answer:
I wonder how much this is also accounted for with the multiple package sign-off work, like if I sign off on the most popular lib that should probably be ok for the smaller ones? |
|
OK, so I take it that this behavior (putting the multiple-files label based on And the further improvement that you're suggesting is to assign this danger level when there are multiple packages that are either untested or critical -- right? |
|
Yes! |
elibarzilay
added a commit
that referenced
this issue
Nov 5, 2020
* 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
Currently, the multiple packages label is set if
dangerLevelis"MultiplePackagesEdited", but this is only fornonTestPackagesTouchedso IIUC, it doesn't look at packages with test. Should it be improved to appear whenever there are multiple packages regardless of test?The text was updated successfully, but these errors were encountered: