-
Notifications
You must be signed in to change notification settings - Fork 3.7k
fix(py-api): hide internal helpers from PyPluginCallApi Python interface #26974
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
base: main
Are you sure you want to change the base?
Conversation
influxdb3_py_api/src/lib.rs
Outdated
| raise RuntimeError(f"unexpected attributes: extras={sorted(extras)}, missing={sorted(missing)}") | ||
| "#; | ||
|
|
||
| let result = system_py::execute_python_with_batch( |
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.
if you switch to execute_schedule_trigger, you'll get the same effect but won't need to construct the empty write_batch.
influxdb3_py_api/src/lib.rs
Outdated
| ))); | ||
| let py_cache = PyCache::new_test_cache(cache_store, "method_visibility".to_string()); | ||
|
|
||
| let query_executor: Arc<dyn QueryExecutor> = Arc::new(UnimplementedQueryExecutor); |
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.
i'd prolly just inline query_executor and py_cache into the call to execute_python_... a matter of taste perhaps, but in a test we can be more concise, especially with setup that is just needed to call the method under test.
probably do the same for db_name (once write_batch is gone) and schema.
influxdb3_py_api/src/lib.rs
Outdated
| use std::time::Duration; | ||
|
|
||
| #[tokio::test] | ||
| async fn py_plugin_call_api_exposes_only_allowed_methods() { |
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.
the test method name should start with test_. We don't always follow this pattern but it makes way easier to recognize when a method is a unit test since in rust tests and prod code are in the same files.
speaking of files. it's unusual to have put this test into lib.rs. It would fit better in system_py.rs. Would you move it there unless there is a reason you can't?
|
The test failure in ci is real: |
philjb
left a comment
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.
nice find about the exposed helper method; pr needs some changes
- see if you can move the test to system_py.rs
- need to fix the test failure ci found
- switch to
execute_schedule_trigger - prefix the test name with
test_
|
Your PR description needs to be more accurate too. The only exposed helper is The added test is not an end to end regression test; imo. Again a matter of taste, but i view the pr description as overly verbose (touch grandiose too) as AI generated text often is. At least for the next few months (🤣 😭 ), humans are reading the text still. It pays to be concise (or at least reduce AI verbosity) to save the human time. AI is actually good at reducing its own text, but you have to ask it. "Write my pr description in a concise and technical prose". How I would edit this: I'm being extra pedantic in your first pr. |
2951459 to
890be88
Compare
890be88 to
ebc2767
Compare
ebc2767 to
ba13e6f
Compare
|
Requested changes are complete. To fix the CI failure, the test was moved to processing_engine because of a dependency on its virtualenv mod. |

PyPluginCallApi exposed internal helper method
log_args_to_stringto plugin code. It shouldn't have been part of the plugin call apiPyPluginCallApi.This PR removes the helper from the api and adds a test.