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

Fix reversed ordering of arguments #1648

Merged
merged 1 commit into from
Aug 22, 2023
Merged

Fix reversed ordering of arguments #1648

merged 1 commit into from
Aug 22, 2023

Conversation

mcastorina
Copy link
Collaborator

@mcastorina mcastorina commented Aug 21, 2023

The source manager initialization function was defined as sourceID followed by jobID, while the source initialization function is the reverse. This is confusing and easy to mix up since the parameters are the same type.

This commit adds a test to make sure the source manager initializes in the correct order, but it doesn't prevent the library user to make the same mistake. We may want to consider using different types.

The source manager initialization function was defined as `sourceID`
followed by `jobID`, while the source initialization function is the
reverse. This is confusing and easy to mix up since the parameters are
the same type.

This commit adds a test to make sure the source manager initializes in
the correct order, but it doesn't prevent the library user to make the
same mistake. We may want to consider using different types.
@mcastorina mcastorina requested a review from a team as a code owner August 21, 2023 16:20
@mcastorina
Copy link
Collaborator Author

The code using the library was in the correct order:

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

func(ctx context.Context, jobID, sourceID int64) (sources.Source, error) {

Copy link
Collaborator

@rosecodym rosecodym left a comment

Choose a reason for hiding this comment

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

strong 👍 for separate types

@mcastorina mcastorina merged commit 5cfbde7 into main Aug 22, 2023
9 checks passed
@mcastorina mcastorina deleted the bug-source-job-id branch August 22, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants