-
Notifications
You must be signed in to change notification settings - Fork 24
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
Use .gitignore
syntax for .distignore
#78
Conversation
Using file lists will be the only way to also handle the more complex gitignore rules
|
Sweet, both PRs got merged already! |
Nice! Thanks for working on this, @BrianHenryIE |
…r` PRs have been merged
Odd, wasn't giving an error on my machine, only on GA.
This is working correctly now. The failing workflows are all for PHP <7.1, as expected. The solution to the Linux file exclusions was two part, stop using |
composer.json
Outdated
@@ -13,7 +13,8 @@ | |||
], | |||
"require": { | |||
"php": ">=5.6", | |||
"wp-cli/wp-cli": "^2" | |||
"wp-cli/wp-cli": "^2", | |||
"inmarelibero/gitignore-checker": "dev-master" |
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.
I think we should lock to a specific version before we merge this: inmarelibero/gitignore-checker#11 (comment)
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.
Done.
@BrianHenryIE I ended up taking the hacky route: I created an issue to improve upon this later: wp-cli/.github#72 |
Ugh, I guess |
4617fc2
to
dde3286
Compare
dde3286
to
aca3946
Compare
I came up with a solution for the test workflows, and while it is not pretty, it does the job of not failing the tests in an unexpected way. See discussion here: https://wordpress.slack.com/archives/C02RP4T41/p1695112126777159?thread_ts=1695076992.280459&cid=C02RP4T41 |
Co-authored-by: Daniel Bachhuber <[email protected]>
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 code looks great, @BrianHenryIE .
I flagged a collection of nitpicks that are supposed to get this to the same code style as the rest of the repositories. I think that this repo was still lagging behind, so there still might be inconsistencies everywhere, but we need to start somewhere, right?
Co-authored-by: Alain Schlesser <[email protected]>
Great work, @BrianHenryIE ! |
@schlessera It looks like the I think it makes less sense to disable automatic syncing for this repository. What about instead reading the minimum PHP version from a |
@danielbachhuber I'm wondering if the automatic syncing of |
@schlessera I think I'd prefer to keep it syncing automatically, if possible:
|
I worked on this at WCUS Contributor Day (wp-cli/wp-cli#5832).
Fixes #44.
It was easier than expected to modify the inmarelibero/gitignore-checker library to use
.distignore
instead of.gitignore
. I've opened a PR and it has no breaking changes and is short and easy to read, so hopefully it will be merged. I do notice the maintainer hasn't been active on GitHub in a few months, but he does have 100+ repos, so I'm still optimistic: inmarelibero/gitignore-checker#11There was an additional PR needed where comments and empty lines needed to be ignored: inmarelibero/gitignore-checker#12
So this PR also hasrequire-dev
cweagans/composer-patches to apply the patches for those PRs as we need them (it's a very popular library I've used extensively).Where the diff looks as though Behat tests were deleted in
dist-archive.feature
, they were moved todistignore.feature
(and history preserved) and additional tests written based on the comments in #44. PHPUnit tests were removed because the function being tested has been removed (and the library used in its place).There were no breaking changes with existing tests for
.distignore
rules.inmarelibero/gitignore-checker requires PHP 7.1.