-
Notifications
You must be signed in to change notification settings - Fork 25
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
bugfixes: create_query is_private field name & add optional arguments to run_sql() #103
Conversation
agaperste
commented
Nov 22, 2023
- fixing create_query's is_private param
- adding performance param for run_sql
…formance param for run_sql
Hi @bh2smith @RichardKeo, when you get a chance can you take a look? Tried to address 1 error and 1 request here. I tested manually and both functions work. Thanks in advance! |
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.
Looks good, but what is up with the title of this PR? It's just a bunch of meaningless codes (I'm sure they mean something, but this public repo has no connection to your foreign task manager).
I strongly suggest renaming that before merge. Think about what you would expect people to see in the release notes (e.g. https://github.com/duneanalytics/dune-client/releases/tag/v1.3.0)
Would be nice to remove some pylint ignores in other places too.
disable=fixme,logging-fstring-interpolation | ||
[DESIGN] | ||
max-args=10 |
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.
There are probably a few ignore statements that can now be removed.
@@ -33,7 +33,7 @@ def create_query( | |||
payload = { | |||
"name": name, | |||
"query_sql": query_sql, | |||
"private": is_private, | |||
"is_private": is_private, |
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.
this should have a test, but this fix is correct
I've fixed the commit message. Yes, we shouldn't pollute the commit history of open source repo with the task-manager issue numbers. |