-
Notifications
You must be signed in to change notification settings - Fork 420
refactor: pr feedback #6406
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
refactor: pr feedback #6406
Conversation
totalSize: number; | ||
done: boolean; | ||
} | ||
type QueryResult = Awaited<ReturnType<Connection['query']>>; |
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.
derive the type from what's calling it instead of redefining it. The type is also better (no any) which was causing a lot of TS warnings.
We want to be getting stricter, not looser, with TS.
* | ||
* @returns Promise resolving to the configured limit number, or undefined if no limit is set. | ||
*/ | ||
private async getMaxFetch(): Promise<number | 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.
doesn't use anything from the class
// Ensure the first record exists and is an object | ||
const firstRecord = records[0]; | ||
if (!firstRecord || typeof firstRecord !== 'object') { | ||
if (!isRecord(firstRecord)) { |
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.
refactored out some repetition to a function
/** | ||
* Converts query records to CSV format | ||
*/ | ||
export const convertToCSV = (records: any[]): string => { | ||
export const convertToCSV = (records: QueryResult['records']): string => { |
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.
getting rid of any
.join(','); | ||
}); | ||
const rows = records | ||
.filter(isRecord) |
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.
removing blank rows
@@ -303,7 +274,7 @@ export const escapeCSVField = (field: string): string => { | |||
/** | |||
* Formats a field value for CSV export | |||
*/ | |||
export const formatFieldValue = (value: any): string => { | |||
export const formatFieldValue = (value: unknown): string => { |
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.
unknown
lets you narrow the type (ex: not null, not undefined, is object, etc). That'll help TS make sure we didn't miss any possibilities.
any
just stays super wide no matter what.
@@ -149,7 +149,7 @@ describe('DataQuery Pure Functions', () => { | |||
}); | |||
|
|||
it('should filter out attributes field', () => { | |||
const records = [{ Id: '001', Name: 'Test1', attributes: { type: 'Account' } }]; | |||
const records = [{ Id: '001', Name: 'Test1', attributes: { type: 'Account', url: 'https://example.com' } }]; |
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.
stricter types do help write more realistic test cases
}); | ||
it('should handle malformed records with undefined first record', () => { | ||
const records = [undefined, { Id: '001', Name: 'Test' }]; | ||
// @ts-expect-error - bad input |
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.
for when you know you're doing something the compiler won't allow, but maybe you want to test your very defensive handling
…ore (#6400) * feat: execute soql query commands don't rely on CLI anymore * chore: pr comments. max fetch, refactor for testing, don't steal focus * feat: execute soql query commands don't rely on CLI anymore * chore: pr comments. max fetch, refactor for testing, don't steal focus * chore: config aggregator * chore: func and tests * refactor: pr feedback (#6406) --------- Co-authored-by: Shane McLaughlin <[email protected]>
No description provided.