Skip to content
This repository has been archived by the owner on Aug 22, 2022. It is now read-only.

Allow the user to exclude folder paths #108

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow the user to exclude folder paths #108

wants to merge 2 commits into from

Conversation

A1Liu
Copy link

@A1Liu A1Liu commented Jun 17, 2020

This PR allows the user to use the -exclude flag to exclude directories.

@cornelk
Copy link

cornelk commented Jul 7, 2020

@markbates any chance to get this merged?

@markbates
Copy link
Owner

Can you add tests please?

@A1Liu
Copy link
Author

A1Liu commented Jul 7, 2020

I'm not sure how to test this; could you help walk me through the process?

@jankaszel
Copy link

jankaszel commented Jul 9, 2020

I've worked on a very similar feature as @A1Liu did and added tests—my proposed API change is a bit different, though, as I would recommend against mutating defaultIgnoredFolders. The API will replace the elegant variadic includes parameter, but I couldn't imagine something else:
https://github.com/falafeljan/pkger/blob/f3870921de210862673b62a9a50747a8fd43db66/parser/parser.go#L35-L44

The test looks like this:
https://github.com/falafeljan/pkger/blob/f3870921de210862673b62a9a50747a8fd43db66/parser/parser_test.go#L74-L100

Tests fail though, as they do on the main branch atm. @markbates to me it occurs that there is a bug in pkging/pkgtest that will somehow keep references to the first directory created by pkgtest.NewRef() for subsequent tests. As the first directory will be deleted after the first test passed, subsequent tests will fail due to the missing directory. Didn't find a solution for that bug, but if you comment out line 26 in the above parser_test.go, tests for the excludes argument will pass.

@jankaszel
Copy link

@A1Liu do you intend to update your PR or shall I open my own one?

@A1Liu
Copy link
Author

A1Liu commented Jul 22, 2020

Go for it dude

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants