-
Notifications
You must be signed in to change notification settings - Fork 317
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
Rename function widget #9959
Rename function widget #9959
Conversation
Technically, it's ready on GUI side, but still does not work because of #9960. I could not find any better name for WidgetFunctionName (literally, the widget displays not only name, but also path/self argument). Any suggestions are welcome. |
app/gui2/src/stores/graph/index.ts
Outdated
mapOk(proj.modulePath, normalizeQualifiedName) | ||
: Err('Uknown current module name') | ||
if (!modulePath?.ok) return modulePath | ||
console.log(ptr.module, modulePath.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over console.log
toastError.show(msg) | ||
} | ||
if (result.ok) code.value = `${ctx.selfIdent}.${result.value}` | ||
toastError.reportError(result, 'Applying AI prompt failed') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing else
newName, | ||
) | ||
if (!refactorResult.ok) return refactorResult | ||
displayedName.value = refactorResult.value.newName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not supre sure if it is OK to modify the local state after await
. This introduces a race condition with other changes happening in the document. I think this should only be applied in case displayedName
wasn't changed in the mean time, or not at all. After all, the name change should be propagated through document update anyway.
app/gui2/src/stores/graph/index.ts
Outdated
} | ||
|
||
function getMethodAst(ptr: MethodPointer, module?: Ast.Module): Result<Ast.Function> { | ||
const topLevel = (module ?? syncModule.value)?.root() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a fallback here, I think the module
argument should be required and syncModule.value
should simply be used as a parameter when it's appropriate (i.e. in computed
above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module
cannot be just any module - it should always be a syncModule
or one of its edited states. I'd rather rename parameter name to edit
and keep it optional. It's published as method of "graph" storage, so it should be obvious of what module we want to take a definition.
app/gui2/src/util/toast.ts
Outdated
reportError<T, E>(result: Result<T, E>, preamble?: string) { | ||
if (!result.ok) { | ||
const msg = result.error.message(preamble) | ||
console.error(msg) | ||
this.show(msg) | ||
} | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this. Putting the error check condition into this function forces the preamble
string to be always computed, and it is sort of awkward to use when you have to check for ok
everywhere anyway. I think this should rather receive a ResultError
as argument and just handle printing it. We can always combine this with a function like mapError
on results.
…/rename-functions
…/rename-functions
…/rename-functions
Pull Request Description
Fixes #9790
Adds a new widget type which allows renaming the called function - if we are able to do it.
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.