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 preserving timestamp when the move command is executing #994

Conversation

KostiantynPopovych
Copy link
Contributor

This PR resolves issue: #992.

These test files are pretty similar, I just took the copy method test files as a blueprint, and it seems we can make some refactoring here and there. I'm open to any suggestions!

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 2, 2023

#992 (comment)

@KostiantynPopovych
Copy link
Contributor Author

Hey @RyanZim, I updated the implementation to match the logic that was discussed in thread #992. I refactored a bit methods move & moveSync to match the same style.

@RyanZim
Copy link
Collaborator

RyanZim commented Mar 2, 2023

It's not clear to me why the large refactor is needed; theoretically you only need to set preserveTimestamps in moveAcrossDevice, similarly to how errorOnExist is currently set. What am I missing?

@KostiantynPopovych
Copy link
Contributor Author

@RyanZim Reverted original code, that refactoring might be overengineering for now 🤔

Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

A few nits in the tests; otherwise looks good.

lib/move/__tests__/move-preserve-timestamp.test.js Outdated Show resolved Hide resolved
lib/move/__tests__/move-preserve-timestamp.test.js Outdated Show resolved Hide resolved
lib/move/__tests__/move-sync-preserve-timestamp.test.js Outdated Show resolved Hide resolved
lib/move/__tests__/move-sync-preserve-timestamp.test.js Outdated Show resolved Hide resolved
KostiantynPopovych and others added 4 commits March 3, 2023 13:24
Co-authored-by: Ryan Zimmerman <opensrc@ryanzim.com>
Co-authored-by: Ryan Zimmerman <opensrc@ryanzim.com>
Co-authored-by: Ryan Zimmerman <opensrc@ryanzim.com>
Co-authored-by: Ryan Zimmerman <opensrc@ryanzim.com>
@KostiantynPopovych
Copy link
Contributor Author

Thank you for reviewing it!

@RyanZim RyanZim merged commit 0e7de32 into jprichardson:master Mar 6, 2023
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