Feature/extract method

[CWDN]

Hi,

I originally made a package called php-extract-method, it allows users to select a piece of code and extract it out into another function (idea taken from PHPStorm). This was alright and had basic functionality but there were a few bugs and sometimes the spacing was off. What I've done is take some of that and move it into this package. But I've made many improvements when doing this and completely rewrote the logic for getting the parameters needed.

Planning to deprecate my old package if you are happy with this, as this version is much better.

Improvements

1. It now tries to guess the variables you want to return from the method and adds them to the end of the new method. (It also handles multiple returns by using list function in php
2. Using all your hard work with the php-integrator-base I'm now able to guess the types of the parameters being passed and the return variable. Then use those types on the docblock.
3. Overall improved the consistency when parsing the parameters to set.
4. General performance improvements.

There's still a small feature that I want to add, which is to allow users to set whether they want to pass the parameters in by reference or for it to return the variables like it currently does. But this feature isn't too important so it can be done at a later date.

I'm adding this feature to this package because it seemed to make more sense to me, rather than creating my own package.

Hope this helps and makes sense. If you have any questions please don't hesitate to ask me.

Thanks
Chris

[ghost]

After your latest changes, I took it for a spin for some real-world usage and noticed the following things (open for discussion, of course):

• The docblock contains the correct types for class types, would it be a good idea to also add these types as type hints in the method signature? I also noticed that the types are not relative to the use statements, but that may very well be a bug in the base package or simply a missing feature on my part.
• Not sure about the [description] blocks in the docblock as they are not obligatory and they are not automatically selected with multiple cursors like snippets do. If the user does decide to specify them, he will need to manually remove them anyway, so would it perhaps be better to just leave them out entirely?
• Neat idea (not necessary for this pull request, though): allow specifying the method description in the dialog (prevents having to update the placeholder). Leaving the field empty would not add a description at all (nor a placeholder).

A minor bug: trying to extract a catch block with a type hint will put the catch block variable in a parameter of the extracted method:

try {

} catch (Exception $e) {

}

Regarding the part of the line, I don't really think it will be a problem, but it might be something we can solve: what if you used the end of the selection (i.e. the code to extract) instead of the end of the variable match? Would this generate problems in some situations?

About type hinting: you are right, the type there is obviously DateTime. Unfortunately there is no (or I don't know of any) specification regarding the annotations or when they should take effect in the local scope. They are in fact inline docblocks on the code level that were adapted for this purpose by IDE's such as PHPStorm (I believe the draft PSR-5 even has a note somewhere saying that it does not apply to these types of annotations). I'll create a ticket in the base package to investigate further how this should behave.

[CWDN]

I've fixed the try catch bug, so it should now exclude the exception from the parameters. I've also added the type hints to the parameters. But it's adding the fully qualified namespace as that is what getVariableType returns, which I think is correct for it to return the FQNS as I don't think it is getVariableType responsibility to manage the use statements. Now I saw in your autocomplete package a function that will add the namespace to the use statements if required. I think this would be invaluable and would allow me to just add the class name to the type hints. It could probably go into the base package, which would then allow other packages to use this feature.

With the doc block descriptions, I think that adding more text fields to the modal is going to make it too big and probably won't fit on some monitors. If we were to add a check box to say whether to generate the description placeholders. Then if the user does we replicate the functionality in atom/snippets where you can tab jump between the different markers [1], like what the docblockr package does. I think this would be best, if you agree then I will build it.

Looking at the selection problem again, I remembered that I'm extending their selected range to the start and end column on the start and end rows respectively. The reason I was doing this was so that in case the user didn't select the entire if statement, for example, this would compensate and select the entire line for you. The only kinda of solution I've thought of is having a config option to specify whether to be fuzzy or precise with the selections and then use the end of the selected range when getting variable type. Do you like this solution or is there a better way?

[1] https://github.com/atom/snippets/blob/master/lib/snippet-expansion.coffee

[ghost]

Regarding the types, adding a use statement for them might not be a bad idea. However, I'd like to do some investigation on how to best approach this first (i.e. it may be best to leave this for an additional improvement after this is merged) as automatically adding use statements raises the following questions:

• Do we directly import every type?
• What if an import for A\B already exists, and the type of a parameter is A\B\C, do we still add a use statement for A\B\C, or de we automatically resolve to B\C?

As you indicated, this is something that the base package (even the PHP side) could learn to handle properly in the future.

Adding a checkbox for the descriptions with tab jumping sounds like a good idea. I'm not sure if this will work, but I noticed the standard atom-snippets package [1] provides a service that contains an insertSnippet method that might (hopefully) help you to reuse the existing behavior used in autocomplete-plus and other packages. All of this functionality will hopefully also pave the road for other features such as #11.

I'm also wondering about the automatic selection correction. On one side I like the convenience it offers, but on the other side I feel like the user should clearly indicate what he or she wants to extract (even if it's wrong, there is always the preview window as validation). If we go the automatic correction route (or support it with a configuration option), it should probably also handle situations such as:

$var = "
Multiline string.
The selection starts at this line";
$andEnds = 'here';

Perhaps it's easiest to just force the user to properly select the code? Do you happen to know how IDE's handle this? (Not that we need to necessarily match this behavior, but it's always good to compare.)

[1] https://github.com/atom/snippets

[CWDN]

New

• Added the snippets support, so now we consume the snippets manager.
• Added option to not generate the description placeholders in docblocks
• Grouped checkboxes together in view with labels

Changed

• Removed the assuming selected range and now works off only what the user has selected.

I found that PHPStorm does some validation before extracting the method on whether it's possible to extract the code and if the code being extracted is valid. Maybe this is something to look at in the future?

[ghost]

Thanks for taking the time to improve this. I think I've kept you busy long enough, so I'll merge this now ;-). As you said, we can always improve the remaining details later. Thanks!