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

Remove opinionated rust-analyzer config from astrocommunity.pack.rust #1109

Closed
boraarslan opened this issue Jul 15, 2024 · 6 comments · Fixed by #1111
Closed

Remove opinionated rust-analyzer config from astrocommunity.pack.rust #1109

boraarslan opened this issue Jul 15, 2024 · 6 comments · Fixed by #1111
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@boraarslan
Copy link
Contributor

Is your feature related to a problem?

config = {
rust_analyzer = {
settings = {
["rust-analyzer"] = {
check = {
command = "clippy",
extraArgs = {
"--no-deps",
},
},
assist = {
importEnforceGranularity = true,
importPrefix = "crate",
},
completion = {
postfix = {
enable = false,
},
},
inlayHints = {
lifetimeElisionHints = {
enable = true,
useParameterNames = true,
},
},
},
},
},
},

There are multiple problems with this config.

assist = {
importEnforceGranularity = true,
importPrefix = "crate",
},

This is entirely wrong and useless. These options are not under "assist". The correct namespaces are rust-analyzer.imports.granularity.enforce and rust-analyzer.imports.granularity.group

inlayHints = {
lifetimeElisionHints = {
enable = true,
useParameterNames = true,
},
},

This just makes the inlay hints way too verbose. Most of the time, the lifetime elision hints are not helpful at all and just add noise.

completion = {
postfix = {
enable = false,
},
},

This is just a bad default. I'm not sure why this is disabled.

check = {
command = "clippy",
extraArgs = {
"--no-deps",
},
},

This is controversial, to say the least. Using clippy as the check command causes very long feedback loops on larger projects as it's slower and heftier than cargo check.

Describe the new feature

The default rust-analyzer configuration is adequate for most people. This opinionated configuration should be removed as it doesn't offer any benefits over the default one.

Additional context

No response

@boraarslan boraarslan added the enhancement New feature or request label Jul 15, 2024
@Uzaaft
Copy link
Member

Uzaaft commented Jul 15, 2024

Feel free to open a PR @boraarslan

@Uzaaft Uzaaft added the good first issue Good for newcomers label Jul 15, 2024
@Uzaaft
Copy link
Member

Uzaaft commented Jul 15, 2024

Most of what you point out makes sense. But I find myself disagreeing with the clippy bit. The pack is there to provide a default for new users of astronvim, who might not know how to configure an LSP. It should stay IMO

@boraarslan
Copy link
Contributor Author

@Uzaaft I'm not really opposed to defaulting to clippy; if I were to suggest something, I would've suggested that README should point out the default check command is not being cargo check and what the user should do in case they want to switch to the default behavior. IMO, doing the exact opposite (defaulting to cargo check and adding the steps needed to switch to cargo clippy to the README file) is better, but as I said, I don't have a strong stance on that.

@Uzaaft
Copy link
Member

Uzaaft commented Jul 15, 2024

Let's document the cargo clippy behaviour, I think that makes sense.
Also, you're overestimating peoples ability to read the docs and make changes. 😂

@boraarslan
Copy link
Contributor Author

Ahahahah, maybe you are right. I've updated the PR to include the documentation changes.

@Uzaaft
Copy link
Member

Uzaaft commented Jul 17, 2024

Grazie Mille @boraarslan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants