Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: skip the generate function assist when dealing with an enum variant #11995

Conversation

maartenflippo
Copy link
Contributor

Fixes #11635. Being my first PR, I have gone through the development docs, and hopefully I haven't missed any critical points.

From a style perspective, the check could also be done by adding the following match arm:

Some(hir::PathResolution::Def(hir::ModuleDef::Adt(hir::Adt::Enum(_)))) => return None,

However, this is rather long so I was unsure which one to go for.

@Veykril
Copy link
Member

Veykril commented Apr 15, 2022

The issue didn't ask about us skipping that part, but instead given the capitalization of the name offering to create an enum variant instead.

@maartenflippo
Copy link
Contributor Author

Oh okay, I thought so initially but due to the link to generate_function.rs I figured maybe it is just preventing the assist from being applicable.

I will expand this PR to fully accommodate this feature then. To make sure I am on the right track, is it fair to say this warrants a separate assist, as opposed to modifying the generate_function assist? This would then still need the change already present in the PR.

@Veykril
Copy link
Member

Veykril commented Apr 15, 2022

Ye I think making that a new assist is fine. It doesn't need much of the things generate_function does.

@maartenflippo maartenflippo marked this pull request as draft April 19, 2022 11:43
@bors
Copy link
Contributor

bors commented May 1, 2022

☔ The latest upstream changes (presumably #12118) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines +76 to +79
if let hir::Adt::Enum(_) = adt {
return None;
}

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't disable this assist for enums in general, we should disable it if the type is an enum and the function name is PascalCase, that is the function name starts with an uppercase letter

Copy link
Member

Choose a reason for hiding this comment

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

Likewise the generate_enum_variant assist should only trigger on enums when the function name starts with an uppercase letter

Comment on lines +61 to +66
let range = adt.source(ctx.db())?.syntax().original_file_range(ctx.db());
let file = ctx.sema.parse(range.file_id);
let adt_ast: ast::Enum =
ctx.sema.find_node_at_offset_with_macros(file.syntax(), range.range.start())?;

Some(adt_ast)
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified with

adt.source(ctx.db())?.original_ast_node(ctx.db())?

bors added a commit that referenced this pull request May 22, 2022
Generate enum variant assist

So, this is kind of a weird PR!

I'm a complete newcomer to the `rust-analyzer` codebase, and so I browsed the "good first issue" tag, and found #11635. Then I found two separate folks had taken stabs at it, most recently `@maartenflippo` — and there had been a review 3 days ago, but no activity in a little while, and the PR needed to be rebased since the crates were renamed from `snake_case` to `kebab-case`.

So to get acquainted with the codebase I typed this PR by hand, looking at the diff in #11995, and I also added a doc-test (that passes).

I haven't taken into account the comments `@Veykril` left in #11995, but I don't want to steal any of `@maartenflippo's` thunder! Closing this PR is perfectly fine. Or Maarten could use it as a "restart point"? Or I could finish it up, whichever feels best to everyone.

I think what remains to be done in this PR, at least, is:

  * [x] Only disable the "generate function" assist if the name is `PascalCase`
  * [x] Only enable the "generate variant" assistant if the name is `PascalCase`
  * [x] Simplify with `adt.source()` as mentioned here: #11995 (comment)
  * [ ] Add more tests for edge cases? Are there cases where simply adding one more indent level than the enum's indent level is not good enough? Some nested trickery I'm not thinking of right now?

Anyway. This PR can go in any direction. You can tell me "no, tackle your own issue!" And I'll go do that and still be happy I got to take a look at rust-analyzer some by doing this. Or you can tell me "okay, now _you_ finish it", and I guess I'll try and finish it :)

Closes #11635
@Veykril
Copy link
Member

Veykril commented May 22, 2022

Hey, #12334 build upon your PR and has been merged, keeping your commit and authorship of said commit. I hope you don't mind that since your work done here is still addressed as yours. Thanks for the work you've done!

@Veykril Veykril closed this May 22, 2022
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.

New assist: add enum variant
3 participants