-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Filter out files that has data_store in name when resolving keystore path #3634
Filter out files that has data_store in name when resolving keystore path #3634
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3634 +/- ##
==========================================
- Coverage 37.50% 36.51% -0.99%
==========================================
Files 311 320 +9
Lines 8357 8960 +603
Branches 1295 1432 +137
==========================================
+ Hits 3134 3272 +138
- Misses 5074 5536 +462
- Partials 149 152 +3 |
Code Climate has analyzed commit 93fe3a5 and detected 0 issues on this pull request. View more on Code Climate. |
Performance Report✔️ no performance regression detected Full benchmark results
|
@@ -88,8 +88,8 @@ function resolveKeystorePaths(fileOrDirPath: string): string[] { | |||
if (fs.lstatSync(fileOrDirPath).isDirectory()) { | |||
return fs | |||
.readdirSync(fileOrDirPath) | |||
.map((file) => path.join(fileOrDirPath, file)) | |||
.filter((filepath) => filepath.endsWith(".json")); | |||
.filter((file) => file.indexOf("deposit_data") === -1 && file.endsWith(".json")) |
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.
A Regex expression is typically much faster than indexOf and endsWith and much more idiomatic
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.
A Regex expression is typically much faster than indexOf and endsWith
I think that is debatable. The benchmarks I see (not ones I run myself) suggests this is not the case.
and much more idiomatic
Using regex is definitely more succinct, and since this is not a hot code path, I guess it is fine to go with the regex.
I have updated.
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.
Good point I haven't benchmarked it 😆
Can you add a couple unit tests for resolveKeystorePaths()? |
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.
Looks good! ❤️
Motivation
Prevents lodestar from crashing if deposit data files are encountered when resolving keystore path.
Description
Filtering out file names that contains deposit_data, when solving keystore path so as to prevent crashing on start up.
Closes #3611
Steps to test or reproduce
--importKeystoresPath
flag