Cmdline#7044
Merged
Merged
Conversation
Move the lmfdb_search logic into an importable module (lmfdb/cmdline_search.py) with a main(argv) entry point so it can be unit tested; lmfdb_search becomes a thin wrapper. Bugs fixed (all crashed at runtime): * missing `import os` (used by --output) and `import csv` (used by csv/tsv output on raw tables) * --cols validation referenced `table` before it was assigned * url-style queries against a raw table referenced an undefined `wrapper`; now reported with a helpful error * undefined `limit` in the section cleaners block (should be args.limit) * `info` was undefined for section searches with a json/empty query and a download format; it is now always initialized * json output was rejected for sections and also fell through to the download branch; json now works for all tables and sections * json serialization crashed on Sage Integer/RealLiteral values; now uses Json.prep, matching the LMFDB API * --format pari crashed because the download language key is "gp"; added a mapping Add lmfdb/tests/test_cmdline_search.py covering raw/json/csv/tsv output, section download formats (sage, pari, magma), url and json queries, --cols, --sort, --oneper, --completeness, --output/--force, and error handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add CommandLineSearch.md (linked from the README) describing the
lmfdb_search command-line interface: sections vs. raw tables, the three
query formats, output formats, all options, and worked examples.
While writing the examples, found that --oneper makes psycodict include
extra grouping/sort columns in each record, which made csv/tsv output
crash in csv.DictWriter ("dict contains fields not in fieldnames"). Pass
extrasaction="ignore" so the output is restricted to the projection, and
add a regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lues bug * --random/-r: return a single object chosen uniformly at random from those matching the query, via table.random(). * implement print_help: `lmfdb_search help TABLE` prints the table description and every column description; `help TABLE COL` prints a single column's description (using table.description()/column_description()). * raw SQL (--sql) already worked, but a second raw= search in the same process failed because psycodict's search() has a mutable default raw_values=[] that _build_query appends limit/offset onto, so stale values leak between calls. Pass a fresh raw_values=[] to immunize the CLI, and add tests for raw SQL. * clarify the "json/tsv language support to downloads" TODO: it refers to adding JSON/TSV DownloadLanguage subclasses to lmfdb/utils/downloader.py; until then the CLI produces those formats itself. * document --random, the help subcommand, and raw SQL in CommandLineSearch.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "checking that we didn't miss any test files" lint step counts test files and requires every one to be listed in matrix_includes.json. Add the new test file to the hilbert_modular_forms/sato_tate_groups/tests shard (proddb and devmirror) and bump the expected count from 42 to 43. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add tests for command-line search and fix bugs
|
This looks great and I'm happy to merge it. My only suggestion for improvement would be to find a way to raise an error when a non-existent column name is specified in the search criteria rather than silently ignoring it (I realize this is not trivial to do, but people are going to make this mistake all the time and be confused by it). |
Addresses the review comment (silently-ignored bad column names) and adds url-paste support. Error on unknown search criteria: * section url queries: validate parameter names against the section's search form (the search-array box names plus the common meta parameters), so a typo like degere=4 is an error instead of being silently ignored. Note these are form-field names, not columns (e.g. signature, discriminant). * json queries: validate the keys against the table columns up front (also recursing into $and/$or/$nor), giving a clean error rather than the raw psycodict traceback. Paste a full LMFDB search-results url as the only argument: * splits the url into the section and the search terms, accepting both the production (www.lmfdb.org) and beta (beta.lmfdb.org) hosts; * if the url is a download url (download=1/Submit=...), the download language becomes the output format (gp -> pari, etc.); an explicit --format wins; * rejects non-LMFDB hosts and non-search urls (e.g. object home pages). To support the url download format, --format now defaults to None and is resolved to "raw" (or the url's format) in run(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These attributes are always either absent or a list, so a plain default suffices; drop the redundant `or []`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two fixes to check_query_columns, per review: * recurse on $not (a real psycodict operator) rather than $nor (which is not); * collect the bad keys from the whole query and report them once, instead of raising inside the recursion and showing only one branch's keys. Split out bad_query_columns(query, valid) which returns the offending keys (mirroring psycodict's _columns_searched: recurse through $and/$or/$not, treat value-level $-operators as operators, and check col.subkey jsonb access against col); check_query_columns now dedupes and raises once. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Accept a pasted url without an http(s):// prefix (e.g. www.lmfdb.org/NumberField/?degree=4 or lmfdb.org/...). urlparse puts the host in the path for a scheme-less url, so parse_lmfdb_url now reparses with a scheme added, and the url/section detection uses the host rather than the scheme. * Accept the internal *.lmfdb.xyz servers in addition to *.lmfdb.org, via a shared is_lmfdb_host() helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Error on unknown search criteria; support pasting a full LMFDB url
Member
Author
|
Good suggestion. I also had Claude add a feature where a user can paste a whole URL. |
Member
Author
|
@AndrewVSutherland approved in person. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add script for searching the LMFDB from the command line.