Proposal for RefactoringMethodParameter.

Proposal for RefactoringMethodParameter.

R=brianwilkerson@google.com, paulberry@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=39253

[Brian Wilkerson, 2014-08-13 23:14:29]

LGTM

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2455: <type
name="RefactoringMethodParameter">
Do we need to allow parameters to be marked as 'optional' or 'named'?

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2463: The unique identifier of the
parameter.
Probably need to specify that the id must match the id returned for a proposed
parameter unless it's being added.

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2962: Clients may change order of
proposed parameters, add new ones
"order" --> "the order"

[Paul Berry, 2014-08-13 23:59:08]

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2467: <field name="type">
I think we need something more than just a String for the type, since we need to
support parameters whose type is a function, e.g. the parameter "callback" in
the method "foo" below:

  foo(ReturnType callback(int arg1, int arg2)) {
  }

[scheglov, 2014-08-14 02:35:28]

PTAL

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2455: <type
name="RefactoringMethodParameter">
On 2014/08/13 23:14:29, Brian Wilkerson wrote:
> Do we need to allow parameters to be marked as 'optional' or 'named'?

Acknowledged.
I will add the "kind" field.

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2463: The unique identifier of the
parameter.
On 2014/08/13 23:14:29, Brian Wilkerson wrote:
> Probably need to specify that the id must match the id returned for a proposed
> parameter unless it's being added.

I will document it at the place where client provides these parameters.

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2467: <field name="type">
On 2014/08/13 23:59:08, Paul Berry wrote:
> I think we need something more than just a String for the type, since we need
to
> support parameters whose type is a function, e.g. the parameter "callback" in
> the method "foo" below:
> 
>   foo(ReturnType callback(int arg1, int arg2)) {
>   }

I will add the "parameters" field as in Element.

https://codereview.chromium.org/470723003/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:2962: Clients may change order of
proposed parameters, add new ones
On 2014/08/13 23:14:29, Brian Wilkerson wrote:
> "order" --> "the order"

Done.

[Paul Berry, 2014-08-14 05:50:58]

lgtm

[Brian Wilkerson, 2014-08-14 14:38:08]

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2477: The kind of the parameter.
I think the comments for "kind" and "type" got reversed.

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2487: <ref>String</ref>
Do we want this to be List<RefactoringMethodParameter>, or is that overkill?

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2990: All the trailing parameters
must use the same kind.
I think it should be defined to be an error if the parameters are not valid.

[scheglov, 2014-08-14 15:54:40]

PTAL

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2477: The kind of the parameter.
On 2014/08/14 14:38:08, Brian Wilkerson wrote:
> I think the comments for "kind" and "type" got reversed.

Ouch!
Fixed.

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2487: <ref>String</ref>
On 2014/08/14 14:38:08, Brian Wilkerson wrote:
> Do we want this to be List<RefactoringMethodParameter>, or is that overkill?

We're not going to change them, so we don't need to make it a list.
As I can see it, both "Extract Method" and "Change Method/Function Signature"
will work only with "top-level" parameters.

In the future we could add a separate refactoring, like "Change Parameter
Function Signature".
Maybe even using the same "Change Function Signature" UI (action, dialog), but
with a quite different implementation.

https://codereview.chromium.org/470723003/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2990: All the trailing parameters
must use the same kind.
On 2014/08/14 14:38:08, Brian Wilkerson wrote:
> I think it should be defined to be an error if the parameters are not valid.

Done.

[Brian Wilkerson, 2014-08-14 16:16:12]

LGTM

https://codereview.chromium.org/470723003/diff/40001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/470723003/diff/40001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2993: It is an error if any
non-trailing parameter is of POSITIONAL or
Might be cleaner to say

It is an error if a REQUIRED or NAMED parameter follows a POSITIONAL parameter.
It is an error if a REQUIRED or POSITIONAL parameter follows a NAMED parameter.

(It avoids the question of what a "trailing" parameter is.)

[scheglov, 2014-08-14 16:46:38]

https://codereview.chromium.org/470723003/diff/40001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/470723003/diff/40001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/spec_input.html:2993: It is an error if any
non-trailing parameter is of POSITIONAL or
On 2014/08/14 16:16:12, Brian Wilkerson wrote:
> Might be cleaner to say
> 
> It is an error if a REQUIRED or NAMED parameter follows a POSITIONAL
parameter.
> It is an error if a REQUIRED or POSITIONAL parameter follows a NAMED
parameter.
> 
> (It avoids the question of what a "trailing" parameter is.)

Done.
Thank you!

[scheglov, 2014-08-14 16:47:01]

Committed patchset #4 manually as 39253 (presubmit successful).