Closed florianbrinkmann closed 4 years ago
Merging #170 into master will increase coverage by
0.02%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #170 +/- ##
============================================
+ Coverage 79.63% 79.65% +0.02%
Complexity 397 397
============================================
Files 24 24
Lines 928 929 +1
============================================
+ Hits 739 740 +1
Misses 189 189
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
inc/Export.php | 98.63% <100%> (+0.01%) |
25 <0> (ø) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 706ea69...90fdd8b. Read the comment docs.
I'm assuming that assets/js/editor.blocks.js
doesn't get committed to the repository?
Yes, that’s correct.
Because of that I used the »fix« to automatically compile the JS and push it to another Git repo via CI, to make the files available to Traduttore like they are on the webserver, but there I had problems with the i18n functions that got minified so that Traduttore did not recognize the strings.
And for the source code links in GlotPress, it is nicer to have a link on the source file instead of a minified version :)
Correct, the idea is to have a link to both files so that the JSON file can be generated for the dist file.
I have to check it again, but I think the place of the filter will not work when assets/js/editor.blocks.js
contains multiple files. Isn't Export::map_entries_to_source()
a better place where all the entries are mapped (and merged) to a file?
With multiple files we would need multiple checks in the filter, something like that (worked in my test, the project I am using for testing has multiple JS source files that get imported into one blocks/index.js
):
if ( $file === 'blocks/block-name/index.js' || $file === 'blocks/other-block/index.js' ) {
return 'assets/js/editor.blocks.js';
}
Or we could also check for the project if there is only one dist JS.
I have to check it again, but I think the place of the filter will not work when assets/js/editor.blocks.js contains multiple files. Isn't Export::map_entries_to_source() a better place where all the entries are mapped (and merged) to a file?
I’m not sure, if I understand it correctly, we would need to check the file or project there too, and then change the $source
.
With multiple files we would need multiple checks in the filter
But that means only the strings from the last file will be in the final JSON file since the content doesn't get merged.
Oh, yes, you are right…
I will try to get it working with a filter in the map_entries_to_source()
and update the PR
I moved the filter to the Export::map_entries_to_source()
and it is working now.
Oops, there is a linter warning, checking it
Checks are passing again :)
I removed the JS references from doc and filter name.
Regarding test: Not really, I have no experience in writing tests. I checked the existing tests for the Export
class and understand what is tested there, but I guess it would take me some time to write a test for the filter…
Ah, maybe I got it, I will try to create a test :)
I wrote a test but have problems setting up the testing environment. I created a Gist with the tests/Export.php
file, maybe you could try if the test is working?
https://gist.github.com/florianbrinkmann/9b8c10be4ff40ea57f84b9c80d3d2b66#file-export-php-L143-L228
Thanks!
This looks good at first glance. Feel free to commit it and let the build decide if it's good enough. 😀
:D okay!
Hi, I am not too deep into the code. My question would be, lets say I have the following setup:
src/
backend.js
frontend.js
shared-components/
component.js
component.js is imported from backend.js and frontend.js, so the strings of component would need to end up in two different json files. Not quite sure, but I think, I couldn't configure this with the filter.
@websupporter Good question. For your case a filter for the final $mapping
would be helpful I guess. This would allow you to merge the entries per source.
function filter_mapping( $mapping ) {
$mapping['backend.js'] = array_merge( $mapping['backend.js'], $mapping['component.js'] );
$mapping['frontend.js'] = array_merge( $mapping['frontend.js'], $mapping['component.js'] );
unset( $mapping['component.js'] ); // Remove since part of other sources.
return $mapping;
}
Something like this?
Hi @ocean90,
yes to be able to filter $mapping
at https://github.com/wearerequired/traduttore/blob/master/inc/Export.php#L182 would enable this I think.
Reading the code, I think such a filter would also enable you to overcome another shortcoming I think I just found: Source-Files do not necessarily have the .js
extension, but might be .ts
or .jsx
. Currently those extensions would be mapped to the source value php
and with the proposed filter I could not configure this appropriately. With a general filter
return (array) apply_filters( 'traduttore.map_entries_to_source', $mapping, $entries, $this->project );
I could basically overwrite the whole outcome.
//Edit:
Changed the filter signature: $this->project
instead of $this->project->get_project()
. This way, you would have also the information about the repository, which could be useful if you provide something like a translation config file in your repository.
Good idea to make the filter more general and get it supporting more cases 👍
Do we need both filters or is one for filtering $mapping
enough?
Source-Files do not necessarily have the .js extension, but might be .ts or .jsx.
WP-CLI only supports js and jsx by default. The make-pot command is used here. https://github.com/wp-cli/i18n-command/issues/200 is also related. I think this should be handled in its own issue.
The one proposed by @websupporter should be enough.
I will update my PR
Updated it, now it is using the filter @websupporter proposed
@florianbrinkmann Thank you! Just noticed that the tests aren't passing due to a wrong filter name.
Thanks for merging! And @websupporter for the idea with the better filter position :)
Just noticed that the tests aren't passing due to a wrong filter name.
Oops, sorry for that, a little too much copy and pasting without thinking 🙈 Thanks for creating a fix-PR!
@florianbrinkmann @websupporter @ocean90 There's now a PR in the WP-CLI repo to allow customizing this mapping right when parsing the source files: https://github.com/wp-cli/i18n-command/pull/284
Would that be useful to you? If so, there could be another filter in Traduttore to leverage it.
Description
I have the issue, that JS translations are not loaded correctly, because the path to the JS file in the Git repo that is used for the string extraction does not match the JS that is loaded on the website. For example, I have the
blocks/block-name/index.js
in my Repository, but it is compiled toassets/js/editor.blocks.js
.So when the
assets/js/editor.blocks.js
is loaded, WP does not find the translation, because the MD5 hash was generated fromblocks/block-name/index.js
.How has this been tested?
I created the following filter call to check the functionality. I added the project as a second parameter to the filter, for the case when one needs more information to decide if a replacement is needed or not (and with what to replace).
Types of changes New feature
Checklist: