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

Use .gitignore syntax for .distignore #78

Merged
merged 37 commits into from
Sep 20, 2023

Conversation

BrianHenryIE
Copy link
Member

@BrianHenryIE BrianHenryIE commented Aug 29, 2023

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#11

There was an additional PR needed where comments and empty lines needed to be ignored: inmarelibero/gitignore-checker#12

So this PR also has require-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 to distignore.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.

@BrianHenryIE BrianHenryIE requested a review from a team as a code owner August 29, 2023 00:42
@BrianHenryIE BrianHenryIE marked this pull request as draft August 29, 2023 01:28
@BrianHenryIE
Copy link
Member Author

tar is failing to exclude files on Linux. I tried a few things but it's not working yet. I'll think on it for a while, and hopefully get a local Ubuntu instance set up so I can test changes without needing to run on GitHub Actions.

@swissspidy
Copy link
Member

Sweet, both PRs got merged already!

@danielbachhuber
Copy link
Member

Nice! Thanks for working on this, @BrianHenryIE

@BrianHenryIE BrianHenryIE marked this pull request as ready for review September 12, 2023 03:14
@BrianHenryIE
Copy link
Member Author

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 ^ and $ characters because it's not actually a regex, it's a glob. And use the --anchored parameter so files in subdirectories do not match rules for files whose shorter paths is a subset of the subdir file.

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"
Copy link
Member

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

composer.json Outdated Show resolved Hide resolved
@danielbachhuber
Copy link
Member

@BrianHenryIE I ended up taking the hacky route:

I created an issue to improve upon this later: wp-cli/.github#72

@danielbachhuber
Copy link
Member

Ugh, I guess composer install still fails 😕 Which makes sense...

@schlessera schlessera force-pushed the use-gitignore-syntax branch 2 times, most recently from 4617fc2 to dde3286 Compare September 19, 2023 08:09
@schlessera
Copy link
Member

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 <daniel@bachhuber.co>
Copy link
Member

@schlessera schlessera left a 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?

composer.json Outdated Show resolved Hide resolved
dist-archive-command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
src/Dist_Archive_Command.php Outdated Show resolved Hide resolved
@schlessera schlessera merged commit c8328db into wp-cli:main Sep 20, 2023
32 checks passed
@schlessera
Copy link
Member

Great work, @BrianHenryIE !

@danielbachhuber
Copy link
Member

@schlessera It looks like the testing.yml workflow file got clobbered... 88c5692

I think it makes less sense to disable automatic syncing for this repository. What about instead reading the minimum PHP version from a .env file or similar?

@schlessera
Copy link
Member

@danielbachhuber I'm wondering if the automatic syncing of testing.yml is still needed at all, given that the changes are pretty much all made in reusable-tesing.yml now...

@danielbachhuber
Copy link
Member

@schlessera I think I'd prefer to keep it syncing automatically, if possible:

  • If we disable it on a one-off basis, we'll probably forgot to update it at some point in the future.
  • If we disable it for all repos, I think it introduces entropy to the system.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment