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
refactor: redundancy of creating lint instance #23308
base: main
Are you sure you want to change the base?
Conversation
cli/tools/lint/mod.rs
Outdated
lazy_static! { | ||
static ref LINTER: Mutex<Option<Linter>> = Mutex::new(None); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems redundant, you can create it directly in lint_files
function
let has_error = has_error.clone(); | ||
let linter = create_linter(lint_rules.rules); | ||
let linter = linter.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartlomieju This might works? creating an instance outside the future and cloning it when it is used inside the future.
@@ -218,7 +220,7 @@ async fn lint_files( | |||
|
|||
futures.push({ | |||
let has_error = has_error.clone(); | |||
let linter = create_linter(lint_rules.rules); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was only created once. #21876 is out of date and already solved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry @wooseok123, it looks like the previous code below is only creating the linter once for the run_parallelized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you for checking that.
I think it needs to add more test code though.
Making a seperate PR for that would be better?
Related issue : #21876
I tried to make a singleton of linter instance so that it can be reused. But it's still on progress.