search

kaleidawave / ezno

A JavaScript compiler and TypeScript checker written in Rust with a focus on static analysis and runtime performance
https://kaleidawave.github.io/posts/introducing-ezno/
MIT License
2.3k stars 42 forks source link

[Refactor] Removing most of clippy warnings #88

Closed JulesGuesnon closed 7 months ago

JulesGuesnon commented 7 months ago

Description

This PR applies most of the suggestions made by clippy. There's still some warnings that I didn't take care of:

Also, I think this PR could be a good timing to choose a lint level and explicitly add it everywhere

kaleidawave commented 7 months ago

Cool. Changes look good so far!

With the Result<..., ()> those are functions where I haven't quite finished what the error should be. Cool to know they are identified by clippy.

In terms of the too many function parameters. I was considering bundling ThisValue, arguments and other argument kind of things into a struct. That would squash a few parameters into one to fix the lint. If you want to attempt then that would be great 👍

Also slightly more ambitious but I am also thinking about putting Publicity into ProperyKey::String. (You can't have non statically known private keys so should be fine to not be on PropertyKey::Type). That should solve the property key too many parameters.

Don't have to and could merge now if you want but you definitely seem capable :)

In terms of lint levels, do you have any suggestions. I think there is a recently added way to specify them in Cargo.toml. Did you encounter a common rule that you think or should be changed from default? Any other changes to the current default config? I haven't too much experience with clippy to know :)

kaleidawave commented 7 months ago

Going to merge this now as I think it is important is added asap. The other points can be addressed in the future with other PRs

JulesGuesnon commented 7 months ago

Hey @kaleidawave ! Sorry for the delay, I've been quite busy last days so I couldn't have a look to your feedbacks. I'll be busy until the end of the week I think, but I'll open another PR to address everything you mentioned when I can

kaleidawave commented 7 months ago

No worries, I thought so 👍. Wanted to work on the project today so was keen to have the improvements in today to not end up with a bunch of merge conflict.