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

spec: move project saving to within the copy function. #120

Merged
merged 2 commits into from
May 21, 2024

Conversation

ashdnazg
Copy link
Contributor

This prevents the mistake where a test doesn't do anything because we
forgot to call the save function.

This prevents the mistake where a test doesn't do anything because we
forgot to call the save function.
@ashdnazg ashdnazg requested a review from barakwei May 15, 2024 09:19
@barakwei
Copy link
Member

This feels a bit weird. The thing is that you're using cp to copy the file, so you have to save the file before copying. Now that you pass the project, you can probably just project.save(new_path) and you'll have all the new state in the project.
The implicit save to the original project is a bit unexpected, but I guess that if that's being done in most of the cases, let's do it.

This prevents the mistake where a test doesn't do anything because we
forgot to call the save function.
@ashdnazg
Copy link
Contributor Author

Good point, I wasn't aware of the argument to save.
F&Sed

Copy link
Member

@barakwei barakwei left a comment

Choose a reason for hiding this comment

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

GG!

@barakwei barakwei merged commit 8bb1446 into Lightricks:main May 21, 2024
4 checks passed
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