Skip to content
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

[NITPICK] From args list to record args RecordPositionFactory #10215

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

prastoin
Copy link
Contributor

@prastoin prastoin commented Feb 14, 2025

Introduction

Just some nitpicking while debugging a bug that wasn't one

@prastoin prastoin marked this pull request as ready for review February 14, 2025 10:43
@prastoin prastoin self-assigned this Feb 14, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR refactors the RecordPositionFactory.create() method to use named object parameters instead of positional arguments, improving code readability and maintainability.

  • Changed create() method signature in /packages/twenty-server/src/engine/api/graphql/workspace-query-runner/factories/record-position.factory.ts to use a single RecordPositionFactoryCreateArgs type
  • Updated all calling code in record-position-backfill-service.ts and query-runner-args.factory.ts to use the new object parameter structure
  • Modified test cases in record-position.factory.spec.ts to reflect the new argument structure while maintaining test coverage
  • Improved type safety by making parameters more explicit through destructuring

4 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

value,
objectMetadata,
workspaceId,
});

expect(result).toEqual(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Test assumes position will be 0 but this depends on the mocked position value (1) from line 19. Consider making the test more explicit about this assumption.

@prastoin prastoin marked this pull request as ready for review February 14, 2025 11:09
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

(updates since last review)

No major changes found since last review. The additional files provided show the implementation details and tests for the previously reviewed changes, but don't introduce any new significant modifications to the codebase.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@prastoin prastoin merged commit 968ad3b into main Feb 14, 2025
35 checks passed
@prastoin prastoin deleted the prastion-nitpick-migration-function-args-to-record branch February 14, 2025 14:43
Copy link
Contributor

Fails
🚫

node failed.

Log

�[31mError: �[39m TypeError: fetch failed
�[90m    at node:internal/deps/undici/undici:12625:11�[39m {
  cause: SocketError: other side closed
  �[90m    at TLSSocket.onSocketEnd (node:internal/deps/undici/undici:9176:26)�[39m
  �[90m    at TLSSocket.emit (node:events:529:35)�[39m
  �[90m    at endReadableNT (node:internal/streams/readable:1400:12)�[39m
  �[90m    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)�[39m {
    code: �[32m'UND_ERR_SOCKET'�[39m,
    socket: {
      localAddress: �[32m'10.1.0.196'�[39m,
      localPort: �[33m54338�[39m,
      remoteAddress: �[32m'104.18.6.191'�[39m,
      remotePort: �[33m443�[39m,
      remoteFamily: �[32m'IPv4'�[39m,
      timeout: �[90mundefined�[39m,
      bytesWritten: �[33m211�[39m,
      bytesRead: �[33m0�[39m
    }
  }
}
danger-results://tmp/danger-results-cb5bad21.json

Generated by 🚫 dangerJS against dc9e7eb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants