Use more deterministic keystore cache path - #5197
Merged
Merged
Conversation
Contributor
Performance Report鉁旓笍 no performance regression detected Full benchmark results
|
nflaig
approved these changes
Feb 22, 2023
nflaig
left a comment
nflaig
left a comment
Member
There was a problem hiding this comment.
LGTM! This should address the issue noted in #5043 (comment)
| ...args, | ||
| onDecrypt: needle, | ||
| cacheFilePath: `${args.importKeystores[0]}.cache`, | ||
| cacheFilePath: join(accountPaths.cacheDir, "imported_keystores.cache"), |
Member
There was a problem hiding this comment.
highly opinionated (feel free to ignore) but I really don't like just having join here, someone has to hover the function to know it from path library or scroll up to the imports to see where this is coming from
since path uses such general names for it's functions I generally like to do path., to make it more obvious we are joining paths here
Suggested change
| cacheFilePath: join(accountPaths.cacheDir, "imported_keystores.cache"), | |
| cacheFilePath: path.join(accountPaths.cacheDir, "imported_keystores.cache"), |
Contributor
Author
There was a problem hiding this comment.
We use destructuring convention all over the code, so seems fine to me. In my preference, I always tend to relate a function to standard library function. If I see a function join I will consider it path.join. If we define a user function which joins string, I would declare it as joinStr to avoid confusion with standard library.
Member
There was a problem hiding this comment.
We use destructuring convention all over the code, so seems fine to me.
I would disagree on when an import should be destructured or not. I do think this should not be based on convention in the code but rather on the design of the library, meaning how did the author/s want you to use it.
In the case of most of the native nodejs libraries those are designed to not use destructuring, one indicator of this are the ambiguous names of functions like for example path has things like delimiter, normalize(path) or parse(path). If we just take parse as an example, this could mean anything, parse a URL, a encoded value, something else? If the authors would have written the library to be more destructuring friendly they would have called this function parsePath.
And if you just look at the docs, for any nodejs library it becomes also clear how they want the libraries to be used, e.g. path, crypto, os
If I see a function join I will consider it path.join.
That might be fine for you as you might know all of the nodejs APIs but for a more unexperienced developer it might not. Functions should clearly convey their purpose without having to think or consider anything and if it requires the code to be more verbose to achieve this it is fine to me.
so seems fine to me.
it definitely is fine in this case, especially since the property right before is called cacheFilePath and the variable is called accountPaths.cacheDir is should be clear that we are joining paths here
This is just more general thing how I think native libraries should be used due to their ambiguous names.
Contributor
There was a problem hiding this comment.
I really like to see path.join over just join too
Contributor
Author
There was a problem hiding this comment.
As I mentioned in some earlier discussion, if any style/pattern can't be enforced by linting then we should keep it flexible.
I am creating a separate issue to enforce not to use the destructuring for the some module imports e.g. path. Once we have that rule, the same style will be enforced everywhere.
Member
|
馃帀 This PR is included in v1.6.0 馃帀 |
twoeths
added a commit
that referenced
this pull request
May 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Use more deterministic keystore cache.
Description
Create a new cache directory and use the keystore cache stored in there.
Closes #5141
Steps to test or reproduce
Ru all tests.