'Sort unit/class members' refactoring implementation.

'Sort unit/class members' refactoring implementation.

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2014-09-26 14:37:41]

brianwilkerson@google.com changed reviewers:
+ lukechurch@google.com

[Brian Wilkerson, 2014-09-26 14:37:41]

The implementation looks good, but I don't think of sorting members as being a
refactoring. In my mind it's much closer to a formatting operation than a
refactoring because it doesn't change the code in any way. The reason that's
important is that it influences presentation in the UX; I don't think most
clients will want to present this functionality with other refactorings. Adding
Luke to get his input.

[scheglov, 2014-09-26 15:24:06]

On 2014/09/26 14:37:41, Brian Wilkerson wrote:
> The implementation looks good, but I don't think of sorting members as being a
> refactoring. In my mind it's much closer to a formatting operation than a
> refactoring because it doesn't change the code in any way. The reason that's
> important is that it influences presentation in the UX; I don't think most
> clients will want to present this functionality with other refactorings.
Adding
> Luke to get his input.

OK

See https://codereview.chromium.org/607813003 for the specification change.

[Brian Wilkerson, 2014-09-26 15:46:37]

https://codereview.chromium.org/609653002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/refactoring/sort_members.dart (right):

https://codereview.chromium.org/609653002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/refactoring/sort_members.dart:83: static
const IMPORT_FILE = const _DirectivePriority('IMPORT_FILE', 2);
I didn't think about this the first time through, but does "FILE" mean 'file:'
or relative? (I'm guessing the latter.) I don't know how important it is, but we
do support a 'file:' URI.

Also, we will need to add support for 'http:' URI's fairly soon, so you might
want to go ahead and add that now.

Finally, when we get support in the VM for custom URI resolvers we will probably
want an UNKNOWN or OTHER category. Not sure it's worth adding at this point, but
it wouldn't hurt to have a catch-all. (It could even replace the file: and http:
categories for now.)

[scheglov, 2014-09-26 16:31:58]

PTAL

https://codereview.chromium.org/609653002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/services/refactoring/sort_members.dart (right):

https://codereview.chromium.org/609653002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/refactoring/sort_members.dart:83: static
const IMPORT_FILE = const _DirectivePriority('IMPORT_FILE', 2);
On 2014/09/26 15:46:37, Brian Wilkerson wrote:
> I didn't think about this the first time through, but does "FILE" mean 'file:'
> or relative? (I'm guessing the latter.) I don't know how important it is, but
we
> do support a 'file:' URI.
> 
> Also, we will need to add support for 'http:' URI's fairly soon, so you might
> want to go ahead and add that now.
> 
> Finally, when we get support in the VM for custom URI resolvers we will
probably
> want an UNKNOWN or OTHER category. Not sure it's worth adding at this point,
but
> it wouldn't hurt to have a catch-all. (It could even replace the file: and
http:
> categories for now.)

Done.

You are right, IMPORT_FILE is actually for relative URIs.
I've added OTHER as a catch-all schemes kind.

[Brian Wilkerson, 2014-09-26 16:52:08]

LGTM

[scheglov, 2014-09-26 17:10:34]

Committed patchset #2 (id:20001) manually as 40718 (presubmit successful).