Literals and operators in Component Browser #12420
Conversation
farmaazon
requested review from
Frizi,
vitvakatu,
kazcw,
AdRiley,
PabloBuchu,
indiv0,
somebody1234 and
MrFlashAccount
as code owners
| input.switchToCodeEditMode() | ||
| return Ok() | ||
| } | ||
| if (component.suggestionId) { |
There was a problem hiding this comment.
suggestionId is a number, and AFAIK the protocol allows it to be 0.
app/ydoc-shared/src/ast/tree.ts
Outdated
| fixBoundaries() { | ||
| const open = this.open | ||
| const close = this.close | ||
| if (open != null && close == null) { |
There was a problem hiding this comment.
If the literal is multiline, close must remain null. It is multiline if open.length > 1.
This function could use some unit tests.
There was a problem hiding this comment.
So I guess setBoundaries also needs this logic?
There was a problem hiding this comment.
setBoundaries is a very low-level operation that may break the validity of the AST in various ways, we might remove it or comment it as only for use in tests.
| let literal: Ast.MutableTextLiteral | Ast.NumericLiteral | undefined = | ||
| Ast.TextLiteral.tryParse(text.value) | ||
| if (literal == null) { | ||
| literal = Ast.NumericLiteral.tryParse(text.value) |
There was a problem hiding this comment.
There is a tryParseWithSign allowing negative numbers. This logic could use unit test coverage.
There was a problem hiding this comment.
The unit tests were once completely removed for input.ts, because CB got huge redesign and we didn't know if it will be stable. For now, I just (re) added tests for cases fixed by this PR.
| watch(toRef(props, 'filtering'), () => (displayedSelectedComponentIndex.value = 0)) | ||
| const filtering = computed(() => { | ||
| const currentModule = projectStore.moduleProjectPath | ||
| return new Filtering(props.filter, currentModule?.ok ? currentModule.value : undefined) |
There was a problem hiding this comment.
This might be a little simpler with unwrapOr.
There was a problem hiding this comment.
currentModule may be undefined, so I wouldn't avoid ?:
| return sourceNodeIdentifier.value ? `${sourceNodeIdentifier.value}.${text}` : text | ||
| return ( | ||
| sourceNodeIdentifier.value ? | ||
| /^[a-zA-Z]/.test(text) ? |
There was a problem hiding this comment.
Technically an identifier can start with a character other than ASCII letters; we could handle that by exposing to WASM an entry point to the lexer so that we can see if the first token is a (lexical) identifier.
I suppose this is ok for an initial implementation but let's add a TODO.
There was a problem hiding this comment.
Well, implementation in rust should be quite easy; I just assumed this is too simple case for it.
But here you pointed me that it's not that simple case.
🧪 Storybook is successfully deployed!📊 Dashboard:
|
farmaazon
changed the title
| ${'"text"'} | ${'"text"'} | ||
| ${"'text'"} | ${"'text'"} | ||
| ${"'text"} | ${"'text'"} | ||
| ${"'''text"} | ${"'''text"} |
There was a problem hiding this comment.
Negative numeric literal case?
| ${'read'} | ${'operator2'} | ${'operator2.read'} | ||
| ${'read "file"'} | ${'operator2'} | ${'operator2.read "file"'} | ||
| ${'+ 2'} | ${'operator1'} | ${'operator1 + 2'} | ||
| ${'+ 3'} | ${'operator2'} | ${'operator2 + 3'} |
There was a problem hiding this comment.
Some more good cases:
- Operators without space
-without space--to make sure we don't treat that as a negative literal here
| @@ -284,8 +285,7 @@ export function useComponentBrowserInput( | |||
| function applySourceNode(text: string) { | |||
| return ( | |||
| sourceNodeIdentifier.value ? | |||
| /^[a-zA-Z]/.test(text) ? | |||
| `${sourceNodeIdentifier.value}.${text}` | |||
| startsWithIdentifier(sourceNodeIdentifier.value) ? `${sourceNodeIdentifier.value}.${text}` | |||
There was a problem hiding this comment.
| startsWithIdentifier(sourceNodeIdentifier.value) ? `${sourceNodeIdentifier.value}.${text}` | |
| startsWithIdentifier(text) ? `${sourceNodeIdentifier.value}.${text}` |
Pull Request Description
Fixes #12372
Screencast.From.2025-03-05.15-34-18.mp4
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
[ ] If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.