-
Notifications
You must be signed in to change notification settings - Fork 93
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
Replace --exclude-from
with --filter
to allow for .gitignore
syntax in .distignore
#148
base: develop
Are you sure you want to change the base?
Conversation
@10up/open-source-practice would be good to get this through review if someone's free/able, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me, but I haven't tested them fully since the testing steps require actual deployment to WP Org.
Hi @claytoncollie, Thanks for working on this and raising the PR. Apologies for the delay in reviewing it. I tried to dry-run this PR action on https://github.com/iamdharmesh/autoshare-for-twitter repo (fork of 10up repo) and it's failing with Thank you. |
@iamdharmesh Thanks for help testing this feature. I made one change to the --filter flag which now allows directory syntax that is used in the distignore file. The static analysis tool also caught an issue that I fixed on line 118. |
@iamdharmesh back to you for review/merge... thanks @claytoncollie! |
Hi @claytoncollie, Apologies for the delay in reviewing again! Thanks for making the changes. The action is now passing without any issues for the existing I made changes to the Thanks again for all your help here. |
Hi @iamdharmesh, I looked at your autoshare repo and noticed that the iamdharmesh/autoshare-for-twitter@cbc8267 https://github.com/iamdharmesh/autoshare-for-twitter/releases/tag/0.0.9 |
@iamdharmesh I am re-reading your original comment about ignoring all text files and then having this single exclusion. Is that possible with |
Hi @claytoncollie, Thanks for checking on this. Yes, I am excluding all text files by default and then specifically including a single file. This can be done using both |
@iamdharmesh Can you test this again, please? I think this is the correct syntax. https://github.com/iamdharmesh/autoshare-for-twitter/releases/tag/0.0.19 |
Hi @claytoncollie, Thanks a lot for working on this. It looks great now and resolves the issue I reported, Great work! While comparing the before-and-after behavior, I noticed that the new sync command does not exclude or delete files that are already committed to SVN, even if they are marked for exclusion. For example, if a plugin repository has Thank you! Some links related to my test:
|
Hi @iamdharmesh I will have to pick this up next week. |
Description of the Change
Swap out the
--exclude-from
flag with the--filter
flag to allow for the full .gitignore syntax to be used in the .distignore file. This change is to provide compatibility for breaking changes in the WP-CLIdist-archive
command.wp-cli/dist-archive-command#78
I think this change might require a release by the dist-archive repository before being needed. At the time of opening this pull request, the last release was 2020 yet more items have been merged into main branch.
Closes #139
How to test the Change
.distignore
to the plugin rootimportant.txt
.distignore
to ignore all.txt
files but to includeimportant.txt
important.txt
is at the destinationChangelog Entry
Credits
Props @claytoncollie
Checklist: