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

Improve theme patterns and styles lookup #320

Merged
merged 6 commits into from
Jul 4, 2022
Merged

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented May 2, 2022

This is in response to https://github.com/wp-cli/i18n-command/pull/312/files#r863120121

Related:

The downside of this change is that now pattern headers are also extracted from files outside the patterns directory. Can we live with that?

@swissspidy swissspidy requested a review from a team as a code owner May 2, 2022 20:20
@swissspidy swissspidy added this to the 2.4.0 milestone May 2, 2022
@swissspidy
Copy link
Member Author

cc @ocean90

@swissspidy
Copy link
Member Author

Perhaps in the extractor we can add a separate check to just extract the strings if the current file is in a patterns directory.

@ocean90
Copy link
Contributor

ocean90 commented May 3, 2022

The downside of this change is that now pattern headers are also extracted from files outside the patterns directory. Can we live with that?

Ah, now it makes sense, why it was done like that. To me this sounds like this task should then only look in patterns and only extract the headers and no other strings. But it seems the current extractor doesn't support this?

I just noticed that we have the same issue with styles from 752a4d4#diff-1197c2ddc1381faef42d8ac1eaf93f93a7b205617cca46cf7611a7462c64f9eeR690, see https://wordpress.slack.com/archives/C02RP50LK/p1651528912931309.

bazza pushed a commit to WordPress/wordpress.org that referenced this pull request May 3, 2022
…ommand too.

Amends [11810].
See [11809].
See wp-cli/i18n-command#320.

git-svn-id: https://meta.svn.wordpress.org/sites/trunk@11811 74240141-8908-4e6f-9713-ba540dce6ec7
@swissspidy
Copy link
Member Author

Yeah that would also work but I think you‘re right, it doesn‘t support that. Plus it would mean potentially unnecessarily scanning a file twice.
Checking for the expected folder structure within the extractor could work for both patterns and styles.

I‘ll neee to experiment with that.

@ocean90
Copy link
Contributor

ocean90 commented May 3, 2022

Plus it would mean potentially unnecessarily scanning a file twice.

That would only be a file_get_contents() + get_file_data_from_string() call right? Sounds okay for me.

What about adding a skip_default option which can be set to true to prevent running

static::fromString( $string, $translations, $options );

@swissspidy
Copy link
Member Author

Tried a different approach now. The downside is that it hardcodes the logic for included folders and file names, but at least it does not mess with the include/exclude options nor scan files twice.

@swissspidy swissspidy changed the title Look for patterns in all directories Improve theme patterns and styles lookup May 3, 2022
# Conflicts:
#	features/makepot.feature
@swissspidy swissspidy merged commit 4bad036 into main Jul 4, 2022
@swissspidy swissspidy deleted the fix/312-followup-patterns branch July 4, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants