-
Notifications
You must be signed in to change notification settings - Fork 20
Add environment variables to control verb AST fallback behavior #219
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
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Co-authored-by: pwwang <[email protected]>
Co-authored-by: pwwang <[email protected]>
…duplication Co-authored-by: pwwang <[email protected]>
|
@copilot We don't need to reimplement the register_verb function. We just need a helper function to get the value of the environment varable, first For example, def _get_verb_ast_fallback_from_env(verb):
# first try DATAR_{verb.upper()}_AST_FALLBAK, then DATAR_VERB_AST_FALLBACK
pass
# then register a verb
@register_verb(ast_fallback=_get_verb_ast_fallback_from_env("mutate"))
def mutate(...):
... |
… wrapper Co-authored-by: pwwang <[email protected]>
Done! I've simplified the implementation to use a simple helper function The wrapper has been removed, and all verbs in dplyr.py, tidyr.py, tibble.py, and misc.py now use the helper function directly as you suggested. Commit: aa8b40d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot We should add this to either mkdocs.yml or some pages that link to this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ENV_VARS.md to the mkdocs navigation (placed after "Options" in the nav menu) and also added a cross-reference link from the options.md page to ENV_VARS.md for better discoverability.
Commit: cfba7c4
Co-authored-by: pwwang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot The integration test is not ideal. We can have some simple verb registered:
@register_verb(ast_fallback=get_verb_ast_fallback("plus")
def plus(x, y):
return x + yThen test it with `DATAR_PLUS_AST_FALLBACK="piping":
# use exec to disable source code being detected at runtime
exec("plus(1, 1)") # error
exec("1 >> plus(1)") # 2You can also add more verbs to test DATAR_VERB_AST_FALLBACK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved the integration tests as suggested! Now using simple registered verbs (plus, minus, multiply, divide, power, modulo) with exec() to disable AST detection at runtime. The tests verify:
- Piping mode (
DATAR_PLUS_AST_FALLBACK="piping") - piping calls work, normal calls return placeholder - Normal mode (
DATAR_MINUS_AST_FALLBACK="normal") - normal calls work, piping calls raise TypeError - Global fallback (
DATAR_VERB_AST_FALLBACK="piping") - applies to multiple verbs - Precedence - per-verb env vars override global setting
All 11 tests passing with 100% coverage on verb_env.py.
Commit: 5cc103d
Co-authored-by: pwwang <[email protected]>
get_verb_ast_fallback()to read environment variablesregister_verbdirectly with the helper functionast_fallback=get_verb_ast_fallback("verb_name")Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.