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

feat:customFuncs add ctx support #219

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

AuroraV
Copy link

@AuroraV AuroraV commented Jul 2, 2023

  I noticed that your func(any, []any) any function does not support the context.Context parameter. I was wondering if it would be possible to update the function signature to support the context.Context parameter. This would greatly enhance the usability of your code and make it more compatible with other libraries.

#218

@wader
Copy link
Contributor

wader commented Jul 2, 2023

For https://github.com/wader/fq i inject context and some other things to functions, so something like this https://go.dev/play/p/fTC2swFweSO another way i think is to have struct with context etc and then have methods on it that you use with gojq.

But note that this only works if you run only one query or the context stays the same.

@AuroraV
Copy link
Author

AuroraV commented Jul 2, 2023

For https://github.com/wader/fq i inject context and some other things to functions, so something like this https://go.dev/play/p/fTC2swFweSO another way i think is to have struct with context etc and then have methods on it that you use with gojq.

But note that this only works if you run only one query or the context stays the same.

Thank you very much for your reply! I'm happy to discover the treasure of the fq.
Regarding your second suggestion, it may not be feasible in my use case as the context is not globally unique, so this approach may not work.I hope it can be flexibly transformed at runtime, like RunWithContext.

@wader
Copy link
Contributor

wader commented Jul 2, 2023

Thanks! ok guess it depends a bit on use case, for fq it seem the performance impact is not that bad to just re-compile and use new wrapper functions for each query

@xakepp35
Copy link

xakepp35 commented Oct 10, 2023

  • there were leftovers, compileCallInternal still used old interface, so it crashed under certain circumstances
  • you hurried with it, maybe it was smarter to keep old interface working as is and just
    • either add opcode opcallctx,
    • either make opcall type-switching (checking which type of function interface is, and returning an error in case of unknown?)
  • supporting both old code: WithFunction + new code through WithCtxFunction
  • offfered that in a discussion: Support for context.Context in func(any, []any) any #218

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