fix: resolve issue with add overwriting existing files #92

[Udayan853]

Changes

• Updated builder.go to ask for confirmation when overwriting existing vault files (skip by default)
• Updated add.go to ignore skipped files
• Updated delete.go to be consistent with new .yaml file convention
• Updated vault files to follow <location>.<filename>.yaml convention to avoid conflicts with duplicate files

Fixes #92

[codecov]

Codecov Report

❌ Patch coverage is 32.14286% with 19 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/manifest/builder.go	0.00%	12 Missing ⚠️
cmd/add.go	0.00%	4 Missing ⚠️
cmd/delete.go	0.00%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[S4tvara]

Hi @Udayan853 ,

please add proper test coverage,
rest looks fine to me

Cheers,
Nilay

[Udayan853]

Hey @SubstantialCattle5

I have added a unit test for confirmation prompt,
However, testing the newly added code block for proper coverage in builder.go, delete.go and add.go would require a significant refactor since the surrounding functions have a lot of side effects
Wouldn't this be considered a separate issue? Or should I proceed to make changes in this PR itself?

[Udayan853]

Ah, nevermind -- I initially thought that 100% coverage would be needed, since only 11.60% is needed I suppose this should be good

[S4tvara]

@Udayan853 ,
thanks for the contribution. In office right now, I'll merge it later after review it

Cheers!
Nilay

[S4tvara]

Hey @SubstantialCattle5

I have added a unit test for confirmation prompt, However, testing the newly added code block for proper coverage in builder.go, delete.go and add.go would require a significant refactor since the surrounding functions have a lot of side effects Wouldn't this be considered a separate issue? Or should I proceed to make changes in this PR itself?

I do need help with total test coverage lemme know if you wanna pick that up!

[Udayan853]

I do need help with total test coverage lemme know if you wanna pick that up!

This has already been taken up in #69.
I'd be happy to help with other unassigned issues though :)

[S4tvara]

@Udayan853,
I’ve reviewed the code and found no issues.
Merging your PR.

Cheers,
Nilay

[S4tvara]

I do need help with total test coverage lemme know if you wanna pick that up!

This has already been taken up in #69. I'd be happy to help with other unassigned issues though :)

@Udayan853,
The issue has been raised but no one has been assigned yet. Send a req over there if you wanna pick it up.