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

Fixed parameter order of InnerJoin interface #478

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

douwinga
Copy link
Contributor

Fix for #477

I went with the non-breaking change way to fix this, but the parameter order seems odd now.

@hishamco
Copy link
Contributor

I think we need to change the implementation not the interface

@douwinga
Copy link
Contributor Author

I agree, I just wasn't sure if that was a good idea since anybody using this will have queries failing after updating. There wouldn't be any indication as to what changed and that they need to flip the parameters. However, if somebody else is using this, you would expect they would have ran into the same problem I did and have reported it. So it is probably a non issue.

@hishamco
Copy link
Contributor

There wouldn't be any indication as to what changed and that they need to flip the parameters.

You are right, but may be we could log it into the release notes

@sebastienros
Copy link
Owner

Please fix the implementation, users should be using the interface anyway.

@douwinga
Copy link
Contributor Author

It looks like I did fix the implementation with the commits from 7 months ago

@sebastienros sebastienros marked this pull request as draft October 24, 2023 15:20
@sebastienros sebastienros marked this pull request as ready for review October 24, 2023 15:20
@sebastienros sebastienros reopened this Oct 24, 2023
@sebastienros sebastienros enabled auto-merge (squash) October 24, 2023 15:21
@sebastienros sebastienros merged commit 1765dbd into sebastienros:main Oct 24, 2023
1 check passed
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.

3 participants