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

Add more standard 'excludes' values #351

Merged
merged 6 commits into from
Feb 15, 2023
Merged

Conversation

cliffordp
Copy link
Contributor

No description provided.

@cliffordp cliffordp requested a review from a team as a code owner February 5, 2023 01:41
@@ -47,7 +47,7 @@ class MakePotCommand extends WP_CLI_Command {
/**
* @var array
*/
protected $exclude = [ 'node_modules', '.git', '.svn', '.CVS', '.hg', 'vendor', 'Gruntfile.js', 'webpack.config.js', '*.min.js' ];
protected $exclude = [ 'node_modules', '.*', '_*', 'vendor', 'Gruntfile.js', 'webpack.config.js', '*.min.js', 'test', 'tests' ];
Copy link
Member

Choose a reason for hiding this comment

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

'.*', '_*' seem a little broad. Could you share more detail about which folders you're solving for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielbachhuber .cache,.github,.parcel-cache,_entry,_test,_temp

Copy link
Member

Choose a reason for hiding this comment

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

@cliffordp Makes sense. I think it'd be better to list those explicitly for now, if it's alright with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any examples of dot files that should be processed, even in edge cases?
Is there an 'include' arg in the MakePot command to override any built-in excludes?

Copy link
Member

Choose a reason for hiding this comment

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

Are there any examples of dot files that should be processed, even in edge cases?
Is there an 'include' arg in the MakePot command to override any built-in excludes?

I don't have any specific examples off the top of my head. Given this is WordPress, I'm sure there's a way for this to be a backwards compatibility break for someone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

better now or you don't want _entry either?

Copy link
Member

Choose a reason for hiding this comment

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

@cliffordp Could you include some functional tests too? Here's the file they should go in and here is some guidance on best practices, if it's helpful.

Here's some prior art you can steal from: https://github.com/wp-cli/dist-archive-command/pull/61/files

Copy link
Member

Choose a reason for hiding this comment

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

But why _entry? Is that a common folder that many projects have and that should most likely never checked? It just seems very project-specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielbachhuber not sure I have time to learn how to do that. I searched for .git since that was an existing excluded item, but I didn't see it.

@swissspidy removed

Copy link
Member

Choose a reason for hiding this comment

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

@cliffordp Sounds good. I added some tests with a58468b

@swissspidy I'm happy with this if you are.

@danielbachhuber danielbachhuber added this to the 2.4.2 milestone Feb 14, 2023
@danielbachhuber danielbachhuber merged commit af1cf2f into wp-cli:main Feb 15, 2023
@cliffordp cliffordp deleted the patch-1 branch February 15, 2023 20:00
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.

3 participants