#8636: fix search/insert for variable popover using optional chaining #8637
Conversation
twschiller
changed the base branch from
main
to
feature/8404-variable-text-toggle
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8637 +/- ##
==========================================
+ Coverage 73.69% 73.71% +0.01%
==========================================
Files 1344 1344
Lines 41566 41596 +30
Branches 7775 7778 +3
==========================================
+ Hits 30633 30663 +30
Misses 10933 10933 ☔ View full report in Codecov by Sentry. |
Playwright test results
Details
Skipped testschrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options |
8636: extract getFullVariableName 8636: handle optional chaining corner cases Clarify method comment
twschiller
force-pushed
the
feature/8636-popover-chaining
branch
from
9a882da
to
3d2f0d6
Compare
| | string | ||
| | number | ||
| | { part: string | number; isOptional: boolean }, | ||
| ): { part: string; isOptional: boolean; isBrackets: boolean } { |
There was a problem hiding this comment.
nit: this would be a useful type to refactor and name to reuse in other places.
| expectMatch(["user"], "user"); | ||
| expectMatch(["users", 0], "users[0]"); | ||
| expectMatch(["users", 0, "name"], "users[0].name"); | ||
| expectMatch(["users", ""], 'users[""]'); | ||
| expectMatch(["names", "Dante Alighieri"], 'names["Dante Alighieri"]'); | ||
| expectMatch( | ||
| ["Ugo Foscolo", "User Location"], | ||
| '["Ugo Foscolo"]["User Location"]', | ||
| ); | ||
| expectMatch(["User List", 100, "id"], '["User List"][100].id'); | ||
| expectMatch( | ||
| ["User List", 100_000_000, "The name"], | ||
| '["User List"][100000000]["The name"]', |
There was a problem hiding this comment.
Tests that verify a wide range of inputs are better suited to using the built-in jest.each. This improves the test output and makes it so that if one expect fails, all others still run.
There was a problem hiding this comment.
Generally agree, was keeping existing style. You'd need to use a table format since the expected value differs per test
src/components/fields/schemaFields/widgets/varPopup/menuFilters.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * Returns true if the value is null or is likely plain text/not a variable. | ||
| */ | ||
| function isTextOrNull(value: string | null): boolean { |
There was a problem hiding this comment.
Since this is specific to variable strings, I'd rename this to something like isTextOrNullVar.
As an aside, if you want to get fancy with TS you can also return a predicate informing the rest of the code that the value is a valid Var. Ex:
type Var = `@${NonEmptyString}`;
const a: Var = "@a";
function isTextOrNullVar(value:string | null): value is Var {
At least this would be nice if defining NonEmptyString weren't so hard in TS:
https://stackoverflow.com/questions/46253340/how-to-write-a-string-type-that-does-not-contain-an-empty-string-in-typescript
Best we can do for string template types is use something like:
type Character = 'a' | 'b' | 'c' | ...;
type NonEmptyString = `${Character}${string}`;
Which I thought type-fest would have, but it doesn't.
There was a problem hiding this comment.
👍 I'm going to punt on the type -- it doesn't add much value relative to effort given the method is only applicable in the context of the module
src/components/fields/schemaFields/widgets/varPopup/likelyVariableUtils.test.ts
Show resolved
Hide resolved
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
|
@fungairino I discovered another corner case (see failing test). I'm going to timebox looking at generating an AST vs. trying to play whack-a-mole with regex rules a bit later this week. We could use an off the shelf parser except for |
|
Update: I created a separate issue for the corner case: #8638 |
What does this PR do?
?during filtering?when a value is selected from the popoverDiscussion
Future Work
Checklist