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

Add autosync before local change for new and edit commands #307

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

patrik-bartak
Copy link
Contributor

@patrik-bartak patrik-bartak commented Jun 27, 2024

Issue #, if available:
#57

Description of changes:
Currently, snippets are lost when autosync is enabled and snippets are added from different machines.
With this change, auto sync is called both before and after a new snippet is added to the local file. This means remote changes will not be overwritten.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@patrik-bartak patrik-bartak changed the title Add autosync before edit for new and edit commands Add autosync before local change for new and edit commands Jun 27, 2024
@patrik-bartak
Copy link
Contributor Author

patrik-bartak commented Jun 27, 2024

To keep it simple I just copied the auto sync call before the local file is changed. The only drawback I see is that there are now two printouts. This is fine but might be confusing.

image image image

Copy link
Collaborator

@RamiAwar RamiAwar left a comment

Choose a reason for hiding this comment

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

LGTM! Simple and well-written, thanks Patrik!

Could you also look into adding tests for this if it's not too much of a hassle? The codebase is still lacking testing wise, so I try to add tests every time I touch a feature.

I don't think we have tests for sync in general yet, so it'll be a nice challenge to see how we'll mock the clients to make them testable locally! Good practice in refactoring code to make it testable.

@patrik-bartak
Copy link
Contributor Author

Yep I can do that

@patrik-bartak
Copy link
Contributor Author

I'm new to open source so I'm happy to practice a bit on a smaller project

@patrik-bartak
Copy link
Contributor Author

patrik-bartak commented Jun 27, 2024

Let's say I want to test func AutoSync(file string) that uses a sync Client. I have two options: refactor to make Client an argument, or using a mocking package like gomock. Is this right or am I missing something?

@RamiAwar
Copy link
Collaborator

RamiAwar commented Jun 28, 2024

Yes exactly. You need to parametrize these dependencies, then you'll be able to replace them with your mocks. By making Client an argument, you can then bring in our own TestClient and pass that in instead and be able to test the logic without calling a real client! I would've linked a nice article on dependency inversion and injection, but I couldn't find one that wasn't worded in a complicated way, and I think you've got the idea honestly.

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.

None yet

2 participants