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

Refactor named parameters feature to allow extensibility #148

Merged
merged 6 commits into from
Apr 7, 2022

Conversation

ipamo
Copy link
Contributor

@ipamo ipamo commented Mar 25, 2022

Refactor parsing of (and access to) named parameters into a single extensible Parameter class. The feature remains the same but the implementation is cleaner.

In addition, the actual Parameter class used by the library may be changed using a setting named DASHBOARD_PARAMETER_CLASS. This allows users of the library to implement custom parameter parsings without changing the default behaviour of the libary.

See issue #149 for extension ideas (%(value)d, %(flag:true)b, etc).

Note: I changed the id of the input HTML fields (from numerical qpX to qp_{parameterName}) so that these fields may be generated directly from instances of the Parameter class.

All tests pass.

@ipamo ipamo changed the title Parameter class Refactor named parameters feature to allow extensibility Mar 25, 2022
Sébastien Hocquet added 3 commits March 25, 2022 21:04
- Parameter name should be omitted only if there is only one parameter
  (not only one provided value)
- Parameters are omitted if they equal their default value
@simonw
Copy link
Owner

simonw commented Apr 7, 2022

This is really neat, thank you.

@simonw simonw merged commit fa839b8 into simonw:main Apr 7, 2022
@simonw
Copy link
Owner

simonw commented Apr 20, 2022

I'm afraid I'm going to have to revert this change back out again - I couldn't get the test suite to pass with it, and when I manually tested I got errors like this one:

CleanShot 2022-04-19 at 17 16 00@2x

I tracked that down to a KeyError being thrown in this block of code:

try:
cursor.execute("BEGIN;")
start = time.perf_counter()
# Running a SELECT prevents future SET TRANSACTION READ WRITE:
cursor.execute("SELECT 1;")
cursor.fetchall()
PARAMETER_CLASS.execute(cursor, sql, parameters)
try:
rows = list(cursor.fetchmany(row_limit + 1))
except ProgrammingError as e:
rows = [{"statusmessage": str(cursor.statusmessage)}]
duration_ms = (time.perf_counter() - start) * 1000.0
except Exception as e:
query_results.append(dict(base_error_result, error=str(e)))

simonw added a commit that referenced this pull request Apr 20, 2022
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.

2 participants