godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.07k stars 69 forks source link

Add 'extract method' as a context action/shortcut for GDScript editor #9361

Open SamDevelopsCode opened 3 months ago

SamDevelopsCode commented 3 months ago

Describe the project you are working on

All projects could benefit from having an 'extract method' added to the in-engine refactoring options. Its a common action for most IDEs and I think it would be a good addition towards bringing the editor to parity, especially if the engine wants to grow to have more professionals use it.

Describe the problem or limitation you are having in your project

Refactoring and creating methods is pretty manual at this point and is a pain point after using other major IDEs.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

It helps automate such a common task in writing code potentially saving time and bugs as well, as it can cut down on copy and pasting.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

extractMethodProposalGif

Once the 'extract method' action is applied, it will take the currently selected section of code, create a new method, and place the new method below the current method, while simultaneously calling the method where the selection was originally selected.

Please disregard the actual code shown. Its in C#. The main thing to takeaway is the process of using 'extract method'.

If this enhancement will not be used often, can it be worked around with a few lines of script?

It will be used often.

Is there a reason why this should be core and not an add-on in the asset library?

It should be core because refactoring is fundamental to most development.

dalexeev commented 3 months ago

This is an interesting proposal, but I see several problems.

Firstly, you can select the code incompletely, so that the beginning/end of the selection will break the expression/statement. The editor should be able to detect this and either display an error to the user or automatically expand the selection.

Second, the selected code may depend on local variables declared above the selected code. In this case, the editor should be able to analyze these dependencies in order to add function parameters and pass them when it is called. Note that with manual refactoring, you might combine multiple arguments used in one expression and pass them as one argument. The editor will likely not be smart enough to pass them all one at a time.

Third, the selected code can modify local variables used below the selected code. Since GDScript does not support passing arguments by reference, in this case the generated function should return the values of the modified variables as an array/dictionary (if there is more than one) and assign them to local variables in the original function after calling the generated function.

Finally, moving the code into a separate function can change the control flow of the original function. If the selected code contains return or continue/break (if you didn't select the entire loop), then the generated function should also return a flag that return/continue/break (and the return value) should be executed after it is called. Also, if the generated function has await, then the original function must also call it with await.

Thus, in the general case, when moving the code into a separate function, the following construction can be formed in its place:

var result = await my_new_func(a, b, c)
if result.need_return_with_value:
    return result.return_value
if result.need_return:
    return
a = result.a
b = result.b
c = result.c
if result.need_continue:
    continue
if result.need_break:
    break    

The selected code can also declare local variables that are used below the selected code. In this case, such variables should be declared after the new function is called (like var a = result.a). Some types (objects, arrays, dictionaries) are passed by reference; they do not need to be passed through the returned array/dictionary, but only if the local variable has not been reassigned in the selected code.

SamDevelopsCode commented 3 months ago

Firstly, you can select the code incompletely, so that the beginning/end of the selection will break the expression/statement. The editor should be able to detect this and either display an error to the user or automatically expand the selection.

You bring up some good points and additional functionality that would greatly benefit the feature. Wouldn't your first point already be handled by the engine in its current state?

  1. If the user were to mess up and select only a partial piece of code, and it did result in a broken expression/statement, this should already be handled by the engine currently. As you would have a function call where it shouldn't be. Not sure any additional error handling would be needed as is.