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 issue with overridden default options #46

Merged
merged 1 commit into from
May 7, 2024

Conversation

ianthpun
Copy link
Contributor

@ianthpun ianthpun commented May 7, 2024

updated the options call so that it adds the default first, then adds the options after so the default doesnt override the incoming options (prepend vs append)

@ianthpun ianthpun changed the base branch from main to feature/stable-cadence May 7, 2024 17:09
@ianthpun ianthpun requested a review from turbolent May 7, 2024 17:17
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Can you please add a test case that verifies the behaviour?

@ianthpun
Copy link
Contributor Author

ianthpun commented May 7, 2024

@turbolent Im trying to see if its possible, but i think we're going to have the same issue as I've outlined here

https://github.com/onflow/flow-go-sdk/pull/636/files#diff-04c09091a8863f8893734eb49df86d6f6e59ff92255cbe7461f3107d7d6e8d39R67-R80

We can confirm that the length is the same, but due to how function signatures are compared, I haven't been able to figure out how to compare functions being called here. I think the most ideal solution would be to see all dialOptions set and just compare the output there. Let me think about this for a little bit

@ianthpun
Copy link
Contributor Author

ianthpun commented May 7, 2024

Really culdnt find a way to test this unfortunately

@ianthpun ianthpun merged commit 89ec9a0 into feature/stable-cadence May 7, 2024
2 checks 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