Repository navigation
feat(ui): Calculate Field suggests fields, functions, variables and keywords as the expression is typed (#1240) - #2040
Conversation
…eywords as the expression is typed (opengeos#1240)
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let chars: Vec<char> = text.chars().collect(); | ||
| let (Some(before), Some(mut after)) = (chars.get(..offer.start), chars.get(offer.end..)) else { return }; | ||
| let put = match of { | ||
| Item::Field(name) => match offer.quote { |
There was a problem hiding this comment.
Bug (confidence: high on the output, medium on how often it happens): a !-opened field (!Ar) is accepted as !name!, but the lexer ends a ! name at the next ! (query.rs, the '!' arm). A field whose name contains ! (e.g. Area!) is therefore written as !Area!!, which reads as Area followed by an unterminated !. The expression then fails with an error instead of the field being used.
Fall back to the quoted form (field_ref) for such names:
| Item::Field(name) => match offer.quote { | |
| Item::Field(name) => match offer.quote.filter(|q| *q != '!' || !name.contains('!')) { |
A regression test with a !-named field would pin this down.
| } | ||
|
|
||
| /// What `item` (as shown) of `offer` stands for. | ||
| fn find<'a>(offer: &'a Offer, item: &str) -> Option<&'a Item> { |
There was a problem hiding this comment.
Bug (confidence: medium): find maps a shown string back to its Item by string equality, and Items are not unique by string. A dataset field named exactly like a keyword or variable (e.g. a field NOT or IN, or $area) is listed before the keyword with the same text. Highlighting the keyword and pressing Enter then resolves to the field and writes "NOT", so the keyword can't be picked. The same lookup feeds the icon and the help.
The fix is to carry the index of the Item through keys/show (or to make the shown strings unique) instead of looking it up again by text.
| } | ||
| of.extend(c.keywords.iter().filter(|k| fits(k.word())).map(|k| Item::Keyword(*k))); | ||
| } | ||
| of.truncate(MAX_ITEMS); |
There was a problem hiding this comment.
Quality (confidence: high): the list is cut silently at 50 items. The doc on MAX_ITEMS says it "scrolls", which is true only up to 50. Fields are added first, so on a table with 50 or more fields, Ctrl+Space shows fields only and no functions, variables or keywords are offered. Typing a prefix avoids this, but nothing says the list was cut. Either lift the cap (the shared list already scrolls) or fix the doc comment, and add a test with 50+ fields.
Code reviewBugs
Security
Performance
Quality
AGENTS.md
No prior review threads existed, so no earlier concerns were skipped. Three inline comments were posted. Change sizeCompared with main (merge base 3da536e to PR head 30e0c4b).
|
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
Summary
Part 3b1 of #1240: the first expression box with suggestions, in Calculate Field.
exprcomplete::expr_editbox shows the shared list from refactor(ui): the Script's completion list and its help move to a shared module (#1240) #2016 under the cursor, with what the language takes there as the parser reads it (query::complete_at, feat(data): complete_at reads what may come next at a point of an expression, from the parser itself (#1240) #1999): the dataset's fields, the functions, the variables and the keywords that start with what's typed, fields first. Nothing is listed by hand, so whatever the language learns is offered too.'text'; field values come in a later part."Pop Est"), or in the quote it was begun with; a function with its bracket open (round(), or closed when it takes no arguments (pi()); a variable or keyword as it is. It is one undo step.scriptcolor::expr_job), with its translated description and an example; a field's type and alias; what a variable reads or a keyword does.Also, as promised in earlier reviews:
List::keyschecks the box's focus itself (refactor(ui): the Script's completion list and its help move to a shared module (#1240) #2016), and the Script pane's wrapper no longer does.complete_atgives no keywords or symbols, and thesymbolsdoc now says only one spelling of each operator is offered and the(or sign that can start a value isn't listed (feat(data): complete_at reads what may come next at a point of an expression, from the parser itself (#1240) #1999).field_refmoves fromsqlbox.rstoexprcomplete.rsand now quotes every keyword of the language (Kw::ALL), not only nine. A field namedcaseorbetween, added from Labeling's field buttons, is now quoted instead of breaking the expression.Testing
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo nextest run --workspace(6434 passed; the DuckDB tests skipped, they fail on my machine for an unrelated reason) andcargo xtask layers, on the branch rebased on main.field_ref's quoting; the list taking no keys without the focus; a quoted name getting no keywords or symbols incompletepo+ Enter writing"Pop Est"without calculating, then* roofferinground()` and Escape closing only the list.keywords; Ctrl+Space on an empty; Enter calculating once the list is closed; the Script pane's list unchanged.
UI evidence
BEFORE
AFTER
Related issue
Part of #1240