refactor(ng-dev): create enum for target label names #202
Conversation
Instead of hard-coding the label names for the target labels the whole Angular organization uses, we now have an enum for these names, allowing for consistent use of the labels. This is in preparation for adding the proper target labels automatically to staging and cherry-pick release PRs. Previously this was not possible because we still had a possible way to configure custom target labels, and missed such an enum. This new enum came together with some refactorings that cleaned up some of the validation and target label extraction logic. Mostly code has been split up into individual files, allowing for more readable code and less convuluted files.
devversion
added
the
action: review
|
@josephperrott I'm on M1 right now, so cannot run the formatter, but can tomorrow. This is a good chance asking for your thoughts on another release of buildifier. Their master branch adds support for M1, but we miss a release 😄 |
There was a problem hiding this comment.
Overall I think its good, just a few ideas.
An idea after seeing how these pieces work together now, maybe in a different follow up PR. I think we should disentangle pull request merging from pull request validation. I will write up an issue for it to have the discussion there.
ng-dev/pr/merge/target-label.ts
Outdated
| * @throws {InvalidTargetLabelError} Invalid label has been applied to pull request. | ||
| * @throws {InvalidTargetBranchError} Invalid Github target branch has been selected. | ||
| */ | ||
| branches: TargetLabelBranchResult | ((githubTargetBranch: string) => TargetLabelBranchResult); |
There was a problem hiding this comment.
To make this simpler should we just change this to
| branches: TargetLabelBranchResult | ((githubTargetBranch: string) => TargetLabelBranchResult); | |
| branches: (githubTargetBranch: string) => Promise<TargetLabelBranchResult>; |
Since its no longer user configuration that defines TargetLabel. Currently only one of them is a non-function, and we always await the branches value anyway. We could then drop the getBranchesFromTargetLabel function as well.
Pretter works on M1
devversion
force-pushed
the
merge-tool-label-rework
branch
from
c42f9d2 to
6dafe98
Compare
|
@josephperrott Addressed most of the feedback. thanks. Please have another look |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Instead of hard-coding the label names for the target labels
the whole Angular organization uses, we now have an enum for
these names, allowing for consistent use of the labels.
This is in preparation for adding the proper target labels
automatically to staging and cherry-pick release PRs. Previously
this was not possible because we still had a possible way to configure
custom target labels, and missed such an enum.
This new enum came together with some refactorings that cleaned up
some of the validation and target label extraction logic. Mostly code
has been split up into individual files, allowing for more readable code
and less convuluted files.
Note: This also fixes
yarn ng-dev pr check-target-branchesif target labels are disabled.