feat(auto-approve): add sample app repo processes #5053
Conversation
| * @param title the title of the PR | ||
| * @returns whether the old dependency, new dependency, and dependency in the title all match | ||
| */ | ||
| export function doesDependencyChangeMatchPRTitleGo( |
There was a problem hiding this comment.
If this is go-specific, why does this allow passing in a dependency regex? The group logic is tied to positional matcher in the provided regex. Also, if this is only used in one place, keep it local to that one place.
There was a problem hiding this comment.
Done. I still needed to add a dependency regex parameter because there is a "file matching" process. If we ever want to add more specific rules to a different file (not a go.mod), then we might need to pass in a different regex.
| * @param versions the versions object returned from getVersions, getVersionsV2, and getJavaVersions | ||
| * @returns true if the version is only bumped a minor, not a major | ||
| */ | ||
| export function runVersioningValidationWithShaOrRev( |
There was a problem hiding this comment.
The naming of this method does not help readability. A better common naming scheme IMO for something returning a true/false value would be isValidSomething.
There was a problem hiding this comment.
Can we revert these changes or fix the formatting?
| @@ -55,6 +55,8 @@ export interface Versions { | |||
| oldMinorVersion: string; | |||
| newMajorVersion: string; | |||
| newMinorVersion: string; | |||
| oldShaOrRevTag?: string; | |||
There was a problem hiding this comment.
Is there a need to overload this class? Can we have a separate class for a SHA diff. The function that parses the diff could return a Versions | ShaDiff type.
I removed the change to the Node dependency max files, since that is the only backwards-looking fix (everything else is a feature). Let me know if you want me to do more for readability. |
| */ | ||
| public doesDependencyChangeMatchPRTitleGo( | ||
| versions: VersionsWithShaDiff, | ||
| dependencyTitleRegex: RegExp, |
There was a problem hiding this comment.
Isn't this a constant? As before, it's possible that a regex doesn't have all the groups you are expecting.
Another alternative is to reference the instance variable here.
| * that particular file. From there, you will get the previous dependency, new | ||
| * dependency, and previous version number and changed version number. | ||
| */ | ||
| export interface VersionsWithShaDiff { |
There was a problem hiding this comment.
consider extends Versions instead of duplicating the fields
| */ | ||
|
|
||
| export class GoDependency extends BaseLanguageRule { | ||
| classRule = { |
There was a problem hiding this comment.
note for a future possible refactor: this classRule nested type does not help you out. Consider flattening these instance variables. Additionally, if they are not actually configurable, make them constants.
Fixes #5006
cc @bourgeoisor
Will remove the Node Dependency requirement in a F/U PR (the Node team does not need max files)