Skip to content

Show record field names in Enum completion - #3169

Merged
bors[bot] merged 4 commits into
rust-lang:masterfrom
adamrk:show-record-field-names-in-enum-completion
Feb 17, 2020
Merged

bors[bot] merged 4 commits into
rust-lang:masterfrom
adamrk:show-record-field-names-in-enum-completion

Conversation

@adamrk

@adamrk adamrk commented Feb 16, 2020

Copy link
Copy Markdown
Contributor

Adresses #2947.
Previously the details shown when autocompleting an Enum variant would look like the variant was a tuple even if it was a record:
2020-02-16-15:59:32_crop

This change will show the names of the fields for a record and use curly braces instead of parentheses:
2020-02-16-15:33:00_crop

This required exposing the type adt::StructKind from ra_hir and adding a function

kind(self, db: &impl HirDatabase) -> StructKind

in the impl of EnumVariant.

There was also a previously existing function is_unit(self, db: &impl HirDatabase) -> bool for EnumVariant which I removed because it seemed redundant after adding kind.

@adamrk
adamrk requested review from flodiebold and matklad February 16, 2020 15:22
@lnicola

lnicola commented Feb 16, 2020

Copy link
Copy Markdown
Member

Sorry for being nitpicky, but can you add some spaces next to the braces? It bugs me a little.

@adamrk adamrk left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamrk

adamrk commented Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

Sorry for being nitpicky, but can you add some spaces next to the braces? It bugs me a little.

No problem - it's done.

@flodiebold flodiebold left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM up to some nitpicks.

Comment thread crates/ra_hir_ty/src/lower.rs Outdated
let struct_data = db.struct_data(def.into());
if struct_data.variant_data.is_unit() {
return type_for_adt(db, def.into()); // Unit struct
match struct_data.variant_data.kind() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be an if let or if ... == StructKind::Unit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just changed it. Out of curiosity - what makes if let preferable? Just that it's more concise? I would think it a lot of situations match without a default is better because you'll get a compiler error if a new variant is added which forces you to explicitly decide how to handle the new variant. Of course, that doesn't really apply here because StructKind isn't likely to change.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case, as you said, StructKind isn't likely to change, and we just want to handle the unit case as a special case here.

Comment thread crates/ra_hir_ty/src/lower.rs Outdated
let var_data = &enum_data.variants[def.local_id].variant_data;
if var_data.is_unit() {
return type_for_adt(db, def.parent.into()); // Unit variant
match var_data.kind() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as could this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@matklad

matklad commented Feb 17, 2020

Copy link
Copy Markdown
Contributor

bors r=flodiebold

Thanks @adamrk !

bors Bot added a commit that referenced this pull request Feb 17, 2020
3169: Show record field names in Enum completion r=flodiebold a=adamrk

Adresses #2947.
Previously the details shown when autocompleting an Enum variant would look like the variant was a tuple even if it was a record:
![2020-02-16-15:59:32_crop](https://user-images.githubusercontent.com/16367467/74607233-64f21980-50d7-11ea-99db-e973e29c71d7.png)

This change will show the names of the fields for a record and use curly braces instead of parentheses:
![2020-02-16-15:33:00_crop](https://user-images.githubusercontent.com/16367467/74607251-8ce17d00-50d7-11ea-9d4d-38d198a4aec0.png)

This required exposing the type `adt::StructKind` from `ra_hir` and adding a function 
```
kind(self, db: &impl HirDatabase) -> StructKind
```
in the `impl` of `EnumVariant`. 

There was also a previously existing function `is_unit(self, db: &impl HirDatabase) -> bool` for `EnumVariant` which I removed because it seemed redundant after adding `kind`.

Co-authored-by: adamrk <ark.email@gmail.com>
@bors

bors Bot commented Feb 17, 2020

Copy link
Copy Markdown
Contributor

Build succeeded

  • Rust (macos-latest)
  • Rust (ubuntu-latest)
  • Rust (windows-latest)
  • TypeScript

@bors
bors Bot merged commit 0e260aa into rust-lang:master Feb 17, 2020
@adamrk
adamrk deleted the show-record-field-names-in-enum-completion branch February 17, 2020 11:33
@sanbox-irl

Copy link
Copy Markdown

wow, thanks man! thanks for noticing my issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants