-
Notifications
You must be signed in to change notification settings - Fork 14
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.
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
feat: Create new package push upgrade class #690
feat: Create new package push upgrade class #690
Conversation
src/package/package.ts
Outdated
@@ -303,8 +303,8 @@ export class Package { | |||
* | |||
* @param force force a refresh of the package data | |||
*/ | |||
public async getPackageData(force = false): Promise<PackagingSObjects.Package2 | undefined> { | |||
if (!this.packageData ?? force) { | |||
public async getPackageData(): Promise<PackagingSObjects.Package2 | undefined> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not change this method since we don't own it. I'd make the change necessary for this to compile on your local but then not include it in the PR. If it gives us issues when attempting to submit we will have to fix it. Also, if you reverse the order in the if it should stop complaining and you it won't change the functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this gave me issue when I tried to push my code, let me check again if we need to include this in PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I flipped the order, a lot of tests failed. And keeping it as it's would not allow me to push code.
Also I think removing force is similar since it's always false and those calling this method is not changing force param (always calling getPackageData() without param)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing this method, since it's public
is a breaking change, which requires a major version bump
src/package/packagePushUpgrade.ts
Outdated
} | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-shadow | ||
async function query(query: string, connection: Connection): Promise<PackagePushRequestListResult[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change this name to listQuery or queryList. Since we will have other queries in here for report
src/package/package.ts
Outdated
@@ -303,8 +303,8 @@ export class Package { | |||
* | |||
* @param force force a refresh of the package data | |||
*/ | |||
public async getPackageData(force = false): Promise<PackagingSObjects.Package2 | undefined> { | |||
if (!this.packageData ?? force) { | |||
public async getPackageData(): Promise<PackagingSObjects.Package2 | undefined> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing this method, since it's public
is a breaking change, which requires a major version bump
src/package/packagePushUpgrade.ts
Outdated
} | ||
|
||
function getListQuery(): string { | ||
const QUERY = 'SELECT Id, PackageVersion, Status' + 'FROM PackagePushRequest ' + '%s'; // WHERE, if applicable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: can make this inline... return 'SELECT Id...
No description provided.