Run clippy and fmt #471
Run clippy and fmt #471
Conversation
ia0
left a comment
ia0
left a comment
There was a problem hiding this comment.
Thanks for the PR! Although I'm not sure what's the context and motivation for this. I left a few comments where it's not clear to me whether that should be done.
| let file_rule = match rule.file_rule.clone() { | ||
| Some(rule) => rule, | ||
| None => continue, | ||
| let Some(file_rule) = rule.file_rule.clone() else { |
There was a problem hiding this comment.
I'm a bit afraid this forces recent compiler version (and slightly a "breaking" change regarding MSRV). Does clippy really suggests to do this?
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm yeah, this let-else was stabilised in 1.65 rust which would be breaking for old versions.
crates/taplo-wasm/src/lsp.rs
Outdated
| } | ||
| } | ||
|
|
||
| unsafe impl Send for WasmLspInterface {} |
There was a problem hiding this comment.
Why is this safe? js_sys::Function doesn't seem to implement Send.
There was a problem hiding this comment.
Oh actually this isn't safe, I made a mistake thinking the writer also required to implement Send which is not the case. The Sink MessageWriter type doesn't require Send so it might be better not to have Arc<Mutex<>> lock instead replace with more performant Rc and fix clippy suggestions?
| $(#[$attrs])* | ||
| $vis struct $name { | ||
| pub(crate) inner: Arc<$inner>, | ||
| pub(crate) inner: Rc<$inner>, |
There was a problem hiding this comment.
Arc is thread-safe, if threads are not a concern, Rc is better.
There was a problem hiding this comment.
Yeah, was wondering what the intention here was since I'm new to the codebase. It looked like Arc implementation can be replaced with Rc here as we don't enforce Send in the inner types. If these are breaking changes, I can revert these type changes as well as the let else changes which were stabilised in 1.65 rust.
There was a problem hiding this comment.
I would keep Arc to preserve usage in multi-threaded environment (there is some in the LSP and CLI as far as I remember). The benefit of Rc is useless because Taplo does not need performance. The project needs to be easily maintainable, safe, and correct before all.
I would also not use let else just yet. It's also not a huge benefit (actually I'm not even convinced it improves readability at all).
From a general point of view, Taplo is only in maintenance mode, so only critical changes are justified. However, if you have time to invest in the project, then I think it would make sense to make you a maintainer and you would then have more freedom to address larger code changes.
There was a problem hiding this comment.
Sounds good, got sidetracked but originally came for the issue which was resolved in #474 Will close this PR since non critical change
|
Closing as non critical change |
3 participants
Some simple clippy cleanup, adds clippy and fmt to CI
These changes are entirely optional, just enforces some lints and reducing some boiler code