Issue 1157113004: add enum suggestions

Issue 1157113004: add enum suggestions (Closed)

Created:
5 years, 6 months ago by danrubel

Modified:
5 years, 6 months ago

Reviewers:
Paul Berry

CC:
reviews_dartlang.org

Base URL:
git@github.com:dart-lang/sdk.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

add enum suggestions and update LocalDeclarationVisitor to visit enum declarations BUG= R=paulberry@google.com Committed: https://github.com/dart-lang/sdk/commit/0e39092fd0c361c82b6af998edd44b7a6b6aa7c7

Patch Set 2 : remove unused parameter and address comments #

Patch Set 3 : merge #

Created: 5 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+200 lines, -40 lines)			Patch
M	pkg/analysis_server/lib/src/protocol_server.dart	View		2 chunks	+7 lines, -1 line	0 comments	Download
M	pkg/analysis_server/lib/src/services/completion/local_declaration_visitor.dart	View		2 chunks	+3 lines, -2 lines	0 comments	Download
M	pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart	View	1	2 chunks	+25 lines, -27 lines	0 comments	Download
M	pkg/analysis_server/lib/src/services/completion/suggestion_builder.dart	View		2 chunks	+16 lines, -1 line	0 comments	Download
M	pkg/analysis_server/test/completion_test.dart	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/analysis_server/test/services/completion/completion_test_util.dart	View		2 chunks	+22 lines, -1 line	0 comments	Download
M	pkg/analysis_server/test/services/completion/imported_reference_contributor_test.dart	View		4 chunks	+24 lines, -2 lines	0 comments	Download
M	pkg/analysis_server/test/services/completion/local_reference_contributor_test.dart	View		4 chunks	+23 lines, -5 lines	0 comments	Download
M	pkg/analysis_server/test/services/completion/prefixed_element_contributor_test.dart	View		2 chunks	+79 lines, -0 lines	0 comments	Download

Messages

Total messages: 8 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Paul Berry

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart File pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart (right): https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart#newcode390 pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart:390: void declaredEnum(EnumDeclaration declaration) { This method looks almost identical ...

5 years, 6 months ago (2015-05-29 17:55:57 UTC) #3

danrubel

Comments addressed. PTAL https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart File pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart (right): https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart#newcode390 pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart:390: void declaredEnum(EnumDeclaration declaration) { On 2015/05/29 ...

5 years, 6 months ago (2015-06-01 04:26:56 UTC) #4

Comments addressed. PTAL

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart
(right):

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/local_reference_contributor.dart:390:
void declaredEnum(EnumDeclaration declaration) {
On 2015/05/29 17:55:57, Paul Berry wrote:
> This method looks almost identical to the two methods above it.  Would you
mind
> extracting a method with the common parts?

Good point. Will do in a subsequent CL so as not to clutter this one.

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/local_suggestion_builder.dart
(right):

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/local_suggestion_builder.dart:30:
String typeName = enumName != null ? enumName.name : nameForType(returnType);
On 2015/05/29 17:55:57, Paul Berry wrote:
> It's not clear to me why this is necessary.  Won't nameForType do the right
> thing for enums?

Oops. That's left over from an earlier refactoring and should have been removed.
Good catch!

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/local_suggestion_builder.dart:68:
returnType: enumName != null ? enumName.name : nameForType(returnType),
On 2015/05/29 17:55:57, Paul Berry wrote:
> Same question here.

As above, old unused code. Removed.

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/test/se...
File pkg/analysis_server/test/services/completion/completion_test_util.dart
(right):

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/completion_test_util.dart:572:
expect(suggestion.element.kind, protocol.ElementKind.FIELD);
On 2015/05/29 17:55:57, Paul Berry wrote:
> Should this be protocol.ElementKind.ENUM_CONSTANT instead of
ElementKind.FIELD?

Yes. See comment above code. Need to investigate whether we want ENUM_CONSTANT
to match API or FIELD to match engine element model. Will address in subsequent
CL.

Paul Berry

lgtm https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/test/services/completion/completion_test_util.dart File pkg/analysis_server/test/services/completion/completion_test_util.dart (right): https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/test/services/completion/completion_test_util.dart#newcode572 pkg/analysis_server/test/services/completion/completion_test_util.dart:572: expect(suggestion.element.kind, protocol.ElementKind.FIELD); On 2015/06/01 04:26:56, danrubel wrote: > ...

5 years, 6 months ago (2015-06-01 12:46:00 UTC) #5

danrubel

Committed patchset #3 (id:40001) manually as 0e39092fd0c361c82b6af998edd44b7a6b6aa7c7 (presubmit successful).

5 years, 6 months ago (2015-06-01 16:10:53 UTC) #6

danrubel

5 years, 6 months ago (2015-06-04 17:55:52 UTC) #7

danrubel

5 years, 6 months ago (2015-06-04 21:48:14 UTC) #8

Message was sent while issue was closed.

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/test/se...
File pkg/analysis_server/test/services/completion/completion_test_util.dart
(right):

https://codereview.chromium.org/1157113004/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/completion_test_util.dart:572:
expect(suggestion.element.kind, protocol.ElementKind.FIELD);
On 2015/06/01 12:45:59, Paul Berry wrote:
> On 2015/06/01 04:26:56, danrubel wrote:
> > On 2015/05/29 17:55:57, Paul Berry wrote:
> > > Should this be protocol.ElementKind.ENUM_CONSTANT instead of
> > ElementKind.FIELD?
> > 
> > Yes. See comment above code. Need to investigate whether we want
ENUM_CONSTANT
> > to match API or FIELD to match engine element model. Will address in
> subsequent
> > CL.
> 
> Oops.  Not sure how I missed the comment.  Thanks!

https://codereview.chromium.org/1157283004/

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages