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

Git feature #30

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Git feature #30

merged 1 commit into from
Mar 15, 2021

Conversation

cinghiopinghio
Copy link
Contributor

Hi,

This is my attempt to implement the github feature as in #27 .

I added the -g command argument to use git ls-files as a file and folder factory.

It works in my environment and passed all test in your test folder (after adding -g to stowsh in all run scripts).

Yet to be done: add permanently tests to the test suite.

@mikepqr
Copy link
Owner

mikepqr commented Mar 12, 2021

This looks great too! Thanks. I'll take a proper look as soon as I can.

In the meantime, the test system is very hacky but it should be self explanatory. You create a directory for the package and directories for what the target look like once it's installed and after it's uninstalled. In this case you'd presumably want to test .gitignored files and untracked files do not make it into the installation if -g is on. Test coverage isn't great (#21) and I have vague plans to move to a real bash unit test framework, so I won't block this merge on the absence of tests, but it would be great to see them!

@cinghiopinghio
Copy link
Contributor Author

I have added a test for the git feature.
It was actually useful as I could spot a small bug in folders with no git files!
Cheers.

Copy link
Owner

@mikepqr mikepqr left a comment

Choose a reason for hiding this comment

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

Looks great! Just one issue that does need fixing, but should be pretty straightforward. Let me know if you have any trouble with my request.

stowsh Outdated Show resolved Hide resolved
Copy link
Contributor Author

@cinghiopinghio cinghiopinghio left a comment

Choose a reason for hiding this comment

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

Should work now

@cinghiopinghio
Copy link
Contributor Author

I'm not sure if I have any more tasks to do in this "review request" (first timer).

Copy link
Owner

@mikepqr mikepqr left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. Tested on macOS.

I think this is ready to merge once you add the quotes (see inline comment) and squash into a single commit. (I think the standard github workflow for that is something like git rebase -i main to tidy up, and then git push -f once you're done to update this PR. Let me know if you have any difficulties.)

stowsh Outdated Show resolved Hide resolved
@mikepqr mikepqr merged commit 07186af into mikepqr:main Mar 15, 2021
@mikepqr
Copy link
Owner

mikepqr commented Mar 15, 2021

Thank you for your contribution!

mikepqr added a commit that referenced this pull request Mar 15, 2021
Document feature added in #30
@mikepqr mikepqr mentioned this pull request Mar 15, 2021
mikepqr added a commit that referenced this pull request Mar 15, 2021
Document feature added in #30
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