fix(auto-approve): allow Python major dependency updates #4998
Conversation
sofisl
changed the title
sofisl
changed the title
sofisl
changed the title
packages/auto-approve/package.json
Outdated
| @@ -29,6 +29,7 @@ | |||
| "dependencies": { | |||
| "@google-cloud/secret-manager": "^4.1.1", | |||
| "ajv": "^8.11.0", | |||
| "compare-versions": "^6.0.0-rc.1", | |||
There was a problem hiding this comment.
We already have the semver dependency in our tree. Can we re-use that one?
|
|
||
| const isVersionValid = semver.compare(oldNum, newNum) === -1; |
There was a problem hiding this comment.
When reviewing this line, it is not clear, what makes it valid or not. I would have to dig into the implementation of semver.compare.
Consider putting this into a tested helper function that is describing what this check is ("is valid" is not descriptive enough). For example, isNotMajorVersionBump() is readable.
| * @param versions an object containing the previous and newer versions of the package being updated | ||
| * @returns whether the minor version was upgraded. | ||
| */ | ||
| export function isVersionBumped(versions: Versions): boolean { |
There was a problem hiding this comment.
If we don't care what kind a version bump, should we just check if oldVersion != newVersion? Why do we need to a semver comparison between constructed values?
There was a problem hiding this comment.
Or why have this check at all?
There was a problem hiding this comment.
We'd want to confirm that it was increasing. You wouldn't want to autoapprove a rollback, for example.
There was a problem hiding this comment.
For readability, please use semver.gt or semver.lt instead (https://github.com/npm/node-semver#comparison). The semver.compare return value (1, 0, -1) is used for sorting.
| * @param versions an object containing the previous and newer versions of the package being updated | ||
| * @returns whether the minor version was upgraded. | ||
| */ | ||
| export function isVersionBumped(versions: Versions): boolean { |
There was a problem hiding this comment.
For readability, please use semver.gt or semver.lt instead (https://github.com/npm/node-semver#comparison). The semver.compare return value (1, 0, -1) is used for sorting.
chingor13
changed the title
Fixes #4126
cc @parthea