Skip to content
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

fix: rename and fetch_data progress report #12

Closed
wants to merge 2 commits into from

Conversation

ho-229
Copy link
Contributor

@ho-229 ho-229 commented Jun 11, 2024

No description provided.

examples/sftp/src/main.rs Outdated Show resolved Hide resolved
examples/sftp/src/main.rs Show resolved Hide resolved
.unlink(&src.strip_prefix(&base).unwrap())
.map_err(|_| CloudErrorKind::InvalidRequest)?;
}
(false, true) => Err(CloudErrorKind::NotSupported)?, // TODO
Copy link
Owner

Choose a reason for hiding this comment

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

What's wrong with the old impl w/ self.create_dir_all and self.create_file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's simpler way.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree, a lot of the impl can be greatly simplified and probably replaced with a single call to fs::copy, but this functionality shouldn't be removed from the example.

src/filter/proxy.rs Show resolved Hide resolved
@ho-229
Copy link
Contributor Author

ho-229 commented Jun 14, 2024

Hi @ok-nick, any problems left?

@ho-229
Copy link
Contributor Author

ho-229 commented Jun 15, 2024

This fix looks weird but it works well.

Cloud Filter will copy remote files(or directories) to local automatically. However, renamed will not be invoked after rename so we can not remove the src from the remote.

@ok-nick
Copy link
Owner

ok-nick commented Jun 15, 2024

Ah, so when the target isn't in scope and the source is, it will invoke SyncFilter::delete rather than SyncFilter::rename?

If that's the case, I wonder if it'd be better to replace the source_in_scope/target_in_scope fns with a single enum to make it more clear. Something like: RenameAction::MoveInsideFromOutside + RenameAction::MoveInsideFromInside.

@ho-229
Copy link
Contributor Author

ho-229 commented Jun 16, 2024

It will not inoke SyncFilter::delete in this case.

@ok-nick
Copy link
Owner

ok-nick commented Jun 16, 2024

How is the file removed from the remote then, if not invoked by SyncFilter::delete? I also don't understand how SyncFilter::renamed plays a role here?

Also, we should keep the existing functionality for moving a file from outside the sync filter to inside the sync filter rather than keeping it as CloudErrorKind::NotSupported here.

@ok-nick
Copy link
Owner

ok-nick commented Jun 17, 2024

I pushed a branch named update-deps to update all the dependencies, let me know if it works for you #14

@ho-229
Copy link
Contributor Author

ho-229 commented Jun 17, 2024

I think I can't fix this problem, thanks for your reviews

@ho-229 ho-229 closed this Jun 17, 2024
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.

2 participants