Skip to content

Conversation

@Bamboozle-jpg
Copy link

Copy folder implements async recursion which required the async recursion crate : https://crates.io/crates/async-recursion.

Comment on lines +106 to +116
#[op2(async)]
pub async fn op_move_file(#[string] origin: String, #[string] dest: String) -> Result<(), AnyError> {
tokio::fs::rename(origin, dest).await?;
Ok(())
}

#[op2(async)]
pub async fn op_move_folder(#[string] origin: String, #[string] dest: String) -> Result<(), AnyError> {
tokio::fs::rename(origin, dest).await?;
Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be one op

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, this could be just one op

Comment on lines +80 to +99
/**
* Moves file asyncrhonously
* @param {string} origin
* @param {string} dest
* @returns {Promise<void>}
*/
function moveFile(origin, dest) {
return core.ops.op_move_file(origin, dest);
}

/**
* Moves folder asyncrhonously
* @param {string} origin
* @param {string} dest
* @returns {Promise<void>}
*/
function moveFolder(origin, dest) {
return core.ops.op_move_folder(origin, dest);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Im-Beast given that these basically call the same op, should this just be move?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it doesn't make sense to have two different functions which do exactly the same thing.
I also wonder if rename is not a better name, since that's how std::fs method is named

Ok(())
}

use async_recursion::async_recursion;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep imports at the beggining of the file


use async_recursion::async_recursion;
#[async_recursion]
pub async fn op_copy_dir_recurse(origin: String, dest: String) -> Result<(), AnyError> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really an op, prefix functions with op_ only when they use #[op]/#[op2] macros

next.push_str(&itemStr);

// Recursive call to copy over contents of folder being looked at
op_copy_dir_recurse(entry.path().into_os_string().into_string().unwrap(), next).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please re-use wholePathStr here

Comment on lines +106 to +116
#[op2(async)]
pub async fn op_move_file(#[string] origin: String, #[string] dest: String) -> Result<(), AnyError> {
tokio::fs::rename(origin, dest).await?;
Ok(())
}

#[op2(async)]
pub async fn op_move_folder(#[string] origin: String, #[string] dest: String) -> Result<(), AnyError> {
tokio::fs::rename(origin, dest).await?;
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, this could be just one op

Comment on lines +80 to +99
/**
* Moves file asyncrhonously
* @param {string} origin
* @param {string} dest
* @returns {Promise<void>}
*/
function moveFile(origin, dest) {
return core.ops.op_move_file(origin, dest);
}

/**
* Moves folder asyncrhonously
* @param {string} origin
* @param {string} dest
* @returns {Promise<void>}
*/
function moveFolder(origin, dest) {
return core.ops.op_move_folder(origin, dest);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it doesn't make sense to have two different functions which do exactly the same thing.
I also wonder if rename is not a better name, since that's how std::fs method is named

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants