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

update watch output to ignore .git paths #21

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

lpmi-13
Copy link
Contributor

@lpmi-13 lpmi-13 commented Nov 4, 2024

currently, the watch command when pushing content outputs a bunch of text that's confusing if you expect it to only care about non-git directories.

Screenshot from 2024-11-03 13-20-00

This commit ignores those directories when adding paths to the watchlist and also adds a test that currently fails on main but passes with this commit. There's also a test to verify that the listFiles function is ignoring git paths as well.

@lpmi-13
Copy link
Contributor Author

lpmi-13 commented Nov 4, 2024

I noticed there don't seem to be any tests currently in this repo, so if that's the preference, I can rebase out that addition. On the other hand, I would happily add a test step as a github action, and even add a few more tests to this repo if that would be helpful!

@iximiuz iximiuz merged commit 4007bf1 into iximiuz:main Nov 5, 2024
@iximiuz
Copy link
Owner

iximiuz commented Nov 5, 2024

Awesome stuff! Thanks for your contribution! Tests are always welcome, but it's not like having a test would prevent this particular bug. If I forgot to update the listDir path when adding the .git ignore logic, I'd also forget to write a test for it. Having more of an e2e test using something like bats might be more helpful - then the test wouldn't make any assumptions about the actual implementation of the push command (e.g., separation on listFiles and listDirs) and only validate that no there is no .git/** entries in the list of watched/pushed files. But such e2e tests are pretty expensive. So, it's always a trade-off.

@lpmi-13
Copy link
Contributor Author

lpmi-13 commented Nov 5, 2024

Yeah, fair point. I mostly added the test to prove that it was failing without the code change and then just left it in, but you're right about e2e testing.

At any rate, my small 14 inch laptop monitor is very happy about this change, since I use the watch mode very frequently!

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