-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fs/memory-provider #1400
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
fs/memory-provider #1400
Conversation
eaaf551 to
ef5a227
Compare
|
hey @KernelDeimos |
1efc8e7 to
62d5027
Compare
9ad082a to
038eed9
Compare
|
all tests passed, it's ready for review & merge repo: XiaochenCui/puter |
a8c4b91 to
74bffda
Compare
| } | ||
|
|
||
| const path = maybe_path_selector.value; | ||
| let path = maybe_path_selector.value; |
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.
Why was this changed to let? I don't see any assignment to path below this line.
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, it should be const
| throw APIError.create('dest_is_not_a_directory'); | ||
| } | ||
| return top_parent; | ||
| return dir; |
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.
I don't agree with this variable name change. I think dir is too vague. top_parent wasn't great either though; perhaps "immediate_parent" would have been better.
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.
The dir is only the immediate_parent from the caller’s perspective. From this function's perspective, it knows nothing about it (and doesn’t seem to care) 😄 .
The funny thing is, this function can create any directory it’s given, since it uses the super-charged MkTree API:
puter/src/backend/src/filesystem/hl_operations/hl_mkdir.js
Lines 490 to 494 in 1e720a7
| const tree_op = new MkTree(); | |
| await tree_op.run({ | |
| parent: await fs.node(new RootNodeSelector()), | |
| tree: [path], | |
| }); |
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.
but I do agree to rename top_parent -> immediate_parent in the mkdir
(another option is use parent for immediate-parent and ancestors for non-immediate-parent)
| class NodePathSelector { | ||
| class NodeSelector { | ||
| path = null; | ||
| uid = null; | ||
| } |
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.
Is this still required or was this for testing purposes?
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.
It's a personal choice and used in doc comments and type check:
// type check
if ( ! (selector instanceof NodeSelector) ) {
throw new Error('Invalid selector type');
}
/**
* Check if a given node exists.
*
* @param {Object} param
* @param {NodeSelector} param.selector - The selector used for checking.
* @returns {Promise<boolean>} - True if the node exists, false otherwise.
*/
6342b0d to
a358c6c
Compare
8cc2295 to
a87739a
Compare
a87739a to
f3d86ad
Compare

fix #1407