fix: no nullish in template literal #627
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mshanemc
reviewed
Jul 30, 2024
| @@ -320,7 +320,9 @@ export class PackageAncestry extends AsyncCreatable<PackageAncestryOptions> { | |||
| // Check to see if the package is an unlocked package | |||
| // if so, throw and error since ancestry only applies to managed packages | |||
| await this.validatePackageType(); | |||
| const normalQuery = `${SELECT_PACKAGE_VERSION} WHERE AncestorId = NULL AND Package2Id = '${this.requestedPackageId}' ${releasedOnlyFilter}`; | |||
| const normalQuery = `${SELECT_PACKAGE_VERSION} WHERE AncestorId = NULL AND Package2Id = '${ | |||
| this.requestedPackageId ?? '' | |||
There was a problem hiding this comment.
where queries and consts are being built, I preserved the existing behavior (with `?? '') to let the server handle the missing value like it currently would
mshanemc
reviewed
Jul 30, 2024
| @@ -205,19 +205,21 @@ export async function createPackageVersionCreateRequest( | |||
| if (hasSeedMetadata) { | |||
| // Zip the seedMetadataFolder folder and put the zip in {packageVersBlobDirectory}/{seedMetadataZipFile} | |||
| Logger.childFromRoot('packageConvert:pollForStatusWithInterval').debug( | |||
| `Including metadata found in '${context.seedmetadata}'.` | |||
| `Including metadata found in '${context.seedmetadata ?? '<undefined seedmetadata>'}'.` | |||
There was a problem hiding this comment.
logs get a more descriptive indicator of what's missing from where
mshanemc
reviewed
Jul 30, 2024
| @@ -145,7 +145,7 @@ export class PackageVersion { | |||
| frequency: Duration.seconds(0), | |||
| timeout: Duration.seconds(0), | |||
| } | |||
| ): Promise<Partial<PackageVersionCreateRequestResult>> { | |||
| ): Promise<PackageVersionCreateRequestResult> { | |||
There was a problem hiding this comment.
the function called at the bottom (byId) claims its return type is PackageVersionCreateRequestResult.
Other consumers of byId also return a PackageVersionCreateRequestResult so I used that.
the very loose Partial would have forced lots of undefined checks.
mshanemc
reviewed
Jul 30, 2024
| @@ -411,8 +411,9 @@ export class PackageVersion { | |||
| } | |||
|
|
|||
| private static getPackage2VersionFields(connection: Connection): string[] { | |||
| const apiVersion = connection.getApiVersion(); | |||
| return Package2VersionFields.filter((field) => (apiVersion > '60.0' ? true : field !== 'ValidatedAsync')); | |||
| return parseInt(connection.getApiVersion(), 10) > 60 | |||
There was a problem hiding this comment.
comparing strings via > works fine until we get to 100, after which 100.0 < 99.0 so use numbers when comparing numbers
mshanemc
reviewed
Jul 30, 2024
| @@ -411,8 +411,9 @@ export class PackageVersion { | |||
| } | |||
|
|
|||
| private static getPackage2VersionFields(connection: Connection): string[] { | |||
| const apiVersion = connection.getApiVersion(); | |||
| return Package2VersionFields.filter((field) => (apiVersion > '60.0' ? true : field !== 'ValidatedAsync')); | |||
There was a problem hiding this comment.
the conditional didn't need to be checked for every field
mshanemc
reviewed
Jul 30, 2024
| @@ -476,34 +477,29 @@ export class PackageVersion { | |||
| * @returns Package2Version | |||
| */ | |||
| public async getData(force = false): Promise<Package2Version> | never { | |||
| let is05i = false; | |||
mshanemc
reviewed
Jul 30, 2024
| label2: BY_LABEL.PACKAGE_VERSION_ID.label, | ||
| }; | ||
| } | ||
| const queryConfig: { id: string; clause: string; label1: string; label2: string } = this.id.startsWith('05i') |
mshanemc
reviewed
Jul 30, 2024
| @@ -202,8 +202,8 @@ export async function getPackageVersionId(versionId: string, connection: Connect | |||
| }); | |||
| } | |||
|
|
|||
| export function escapeInstallationKey(key?: string): Nullable<string> { | |||
| return key ? key.replace(/\\/g, '\\\\').replace(/'/g, "\\'") : null; | |||
| export function escapeInstallationKey(key: string): string { | |||
There was a problem hiding this comment.
every consumer was passing a string
mshanemc
changed the title
cristiand391
approved these changes
Jul 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
updates the deps for all the other PRs.
applies our linter rule ( no nullish in template literals)
reduce
let