Proposal for the 'edit.organizeDirectives' request.

Proposal for the 'edit.organizeDirectives' request.

R=brianwilkerson@google.com
BUG= https://github.com/dart-lang/sdk/issues/23921

Committed: https://github.com/dart-lang/sdk/commit/aa30cad380a352423c8d4abcf35ad00ea76e42d5

[Brian Wilkerson, 2015-07-31 19:47:35]

In general, I like it, but I have two concerns.

The first is the definition of "unused imports". Currently our unused import
hint isn't as complete as we might want for this request (primarily because it's
sensitive to the order of the imports). For example, consider the following:

import 'dart:io' show File;
import 'dart:io';

void main() {
  File f;
  Directory d;
}

Technically, both imports are being used because we consider the first import
that unambiguously defines the name to be the one being used. But if you reverse
the order then the import with a 'show' will be unused. Are we proposing to do
this level of optimization (find the minimum number of imports)?

Also, what about imports that reference a URI that doesn't exist? Assuming that
those are not parse errors (which is what I'd assume if I didn't know how we
currently divide the work between tasks), are those considered unused, or will
they remain in the list (under the assumption that the user will want to fix the
import rather than have it removed)?

Which leads to the second concern: the use of parse errors as a gating factor.
Perhaps we should side-step the question above by not operating on code that
contains any errors or warnings. There are other potential problems we could
avoid by doing so, such as the following example:

import 'dart:io' as io;

main() {
  o.File f;
}

Mistyping the prefix keeps us from resolving 'File', which means that we can't
tell that the import is intended to be used.

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

https://codereview.chromium.org/1258563008/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:1724: <ref>SourceFileEdit</ref>
Do we always want to replace all of the imports as a single edit, or might we
want to optimize the edits by having a small number of less disruptive edits? (I
don't know which is best, I just want to make sure we're thought about it.)

https://codereview.chromium.org/1258563008/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:3491:
<code>ORGANIZE_DIRECTIVES_INVALID_FILE</code>
I'm not sure I like this trend of having a separate error code for each request.
Kind of seems like we could have a single "FILE_NOT_ANALYZED" error code for all
of the "*_INVALID_FILE" codes. The client already knows which request cause the
error. Of course, it would be a breaking change to change the others, and it
would be inconsistent to have one general code and lots of specific codes,
but...

[scheglov, 2015-07-31 20:25:02]

PTAL

Discussed offline, we will try to do the right thing.
We don't specify what it is exactly :-)

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

https://codereview.chromium.org/1258563008/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:1724: <ref>SourceFileEdit</ref>
On 2015/07/31 19:47:35, Brian Wilkerson wrote:
> Do we always want to replace all of the imports as a single edit, or might we
> want to optimize the edits by having a small number of less disruptive edits?
(I
> don't know which is best, I just want to make sure we're thought about it.)

Discussed offline.
A single file edit, with possible multiple edits in that file.

https://codereview.chromium.org/1258563008/diff/1/pkg/analysis_server/tool/sp...
pkg/analysis_server/tool/spec/spec_input.html:3491:
<code>ORGANIZE_DIRECTIVES_INVALID_FILE</code>
On 2015/07/31 19:47:35, Brian Wilkerson wrote:
> I'm not sure I like this trend of having a separate error code for each
request.
> Kind of seems like we could have a single "FILE_NOT_ANALYZED" error code for
all
> of the "*_INVALID_FILE" codes. The client already knows which request cause
the
> error. Of course, it would be a breaking change to change the others, and it
> would be inconsistent to have one general code and lots of specific codes,
> but...

Done.

[Brian Wilkerson, 2015-07-31 20:45:38]

LGTM

[scheglov, 2015-08-03 14:59:29]

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