Skip to content

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

Merged
merged 1 commit into from
Jul 8, 2025
Merged

refactor: pr feedback #6406

merged 1 commit into from
Jul 8, 2025

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented Jul 8, 2025

No description provided.

@mshanemc mshanemc requested a review from a team as a code owner July 8, 2025 20:28
@mshanemc mshanemc requested a review from madhur310 July 8, 2025 20:28
totalSize: number;
done: boolean;
}
type QueryResult = Awaited<ReturnType<Connection['query']>>;
Copy link
Contributor Author

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> {
Copy link
Contributor Author

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)) {
Copy link
Contributor Author

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 => {
Copy link
Contributor Author

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)
Copy link
Contributor Author

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 => {
Copy link
Contributor Author

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' } }];
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@CristiCanizales CristiCanizales merged commit 1301894 into cristi/queryUser Jul 8, 2025
6 checks passed
@CristiCanizales CristiCanizales deleted the sm/pr-feedback branch July 8, 2025 20:52
CristiCanizales added a commit that referenced this pull request Jul 8, 2025
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants