[#163] Added support for translation extractors for JS #164

jcchavezs · 2016-06-10T08:28:53Z

Closes #163.

Ping @willdurand, @JANorman, @monteiro and @stof

jcchavezs · 2016-06-10T08:30:30Z

Filesystem/Filesystem.php

+
+class Filesystem extends BaseFilesystem
+{
+    public function getContents($filepath)


The main purpose for this is to mock the usage of getContents in the tests.

Once you stop mocking the finder (which should not be done if you want the test to actually test that things are working), you won't need to mock getting the file content either

jcchavezs · 2016-06-10T08:38:37Z

retest this please

stof · 2016-06-10T08:58:07Z

Extractor/Extractor.php

+
+    public function __construct(
+        Filesystem $filesystem,
+        Finder $finder


the signature should stay on 1 line

jcchavezs · 2016-06-17T10:37:07Z

There is a problem when dealing with more than one bundle. I will fix it and the checks failure in upcoming commits.

cmfcmf · 2016-08-03T08:17:04Z

Extractor/JsExtractor.php

+
+final class JsExtractor extends Extractor
+{
+    protected $sequence = '\\.trans[Choice]*\\(';


I think this should rather be '\\.trans(?:Choice)?\\('

I can't see the implementation difference. Can @cmfcmf explain it to me?

your regex matches transoichie too, as it allows to put any string containing the letters C, h, o, i, c and e

Sure: Compare your regex:
https://regex101.com/r/9Ng6xR/1
with mine
https://regex101.com/r/wrJ9WE/1

Your regular expression does also match trans... where ... is any combination of the letters C, h, o, i, c, e. Mine looks specifically for Choice, in this order.

Right. Changing it now.

you should also cover this in tests btw

Right. Tests for this are added.

fivetide · 2016-10-17T09:34:39Z

Is there any way one could help with this? I would love to be able to see this merged...

…Coffeescript and the inclusion of phpunit for the tests.

…urces/config/services.xml.

…he extractors stateless.

…r to not having broken inheritances.

jcchavezs · 2016-10-21T09:26:11Z

@willdurand is there any possibility we give up with PHP 5.3? Since now one constraint is Symfony 2.7 which only works with >5.3.9. What do you think?

stof · 2016-10-21T09:29:24Z

@jcchavezs I don't understand how the fact that Symfony 2.7 dropped support for PHP 5.3.8 has anything to do with the PHP 5.4 support.

jcchavezs · 2016-10-21T09:33:52Z

@stof sorry, I meant PHP 5.3. I just fixed my comment.

stof · 2016-10-21T09:46:19Z

well, dropping support for initial PHP 5.3 releases (which were recommended against them since Symfony 2.2 already as they are totally buggy) does not change the fact that Symfony 2 supports PHP 5.3 (the uptodate 5.3 version is 5.3.29).

anyway, dropping support for old PHP versions is out of the scope of this PR

stof · 2016-10-21T09:46:41Z

composer.json

@@ -11,6 +11,7 @@
    }
    ],
    "require": {
+      "symfony/translation": "~2.7|~3.0",


wrong indentation

stof · 2016-10-21T09:47:22Z

composer.json

@@ -20,7 +21,8 @@
        "symfony/yaml": "~2.7|~3.1",
        "symfony/browser-kit": "~2.7|~3.1",
        "symfony/twig-bundle": "~2.7|~3.1",
-        "symfony/phpunit-bridge": "~2.7|~3.1"
+        "symfony/phpunit-bridge": "~2.7|~3.1",
+        "phpunit/phpunit": "~4.8"


you should allow both 4.8 and 5.x if you install it as a dependency, because PHPUnit 4.8 does not officially support PHP 7.

stof · 2016-10-21T09:47:46Z

Tests/Extractor/JsExtractorTest.php

+    /**
+     * @var JsExtractor
+     */
+    private $sut;


extractor would be a better name IMO

sut stands for subject under testing which makes it more clear that this is what we are testing.

stof · 2016-10-21T09:49:46Z

Tests/Extractor/JsExtractorTest.php

+        $finder = $this->prophesize(Finder::class);
+
+        $finder
+            ->files()


please don't mock the finder. This does not allow you to ensure that it is configured properly, as you never ensure that the way you configured it does way you expect it to be. You should not mock what you don't own (if Symfony changes the behavior of the Finder, this test will not warn you about it, as nothing ensures that your mock describes the real behavior)

This is a unit test and Finder is very tight to the implementation and it does not implement any interface and if I don't mock it then I need to provide actual files to test it (why should I do it in a unit test?).

But there is nothing in your test ensuring that the way you configured the finder is actually the right one to find the files you want to find. so your test currently says "if the code calls the Finder in the way it currently calls it, things are working". This is wrong. I does not actually ensure that configuring the Finder this way is what makes things work.
And it does not protect you from changes in the Finder behavior either.

What you need here is an integration test, which could use the real Finder. You should not mock the finder as it is outside your domain boundary. Using a unit test to cover this code is precisely the issue.

stof · 2016-10-21T09:50:30Z

Finder/FinderFactory.php

+
+use Symfony\Component\Finder\Finder;
+
+class FinderFactory


IMO, this class is not necessary

Finder is stateful so on every call to AbstractFileExtractor::extractFiles which calls to AbstractFileExtractor::extractFromDirectory i need to create a new finder because I can not reuse an existing one and the only one way to do that is to use a factory otherwise I need to call the constructor of Finder inside the class which is so bad and make it untestable.

having such a factory is only useful to allow you mocking the Finder in tests, which is already something your test should not do.

It allows me to avoid the instantiation of classes inside my class.

instantiating a class which is not a service is fine.

and btw, you don't avoid instantiating in your class. You just moved it to a different class of yours

stof · 2016-10-21T09:51:45Z

Resources/config/services.xml

@@ -22,5 +22,17 @@
            <argument></argument> <!-- active locales -->
            <argument></argument> <!-- active domains -->
        </service>
+        <service id="bazinga.jstranslation.filesystem" class="Bazinga\Bundle\JsTranslationBundle\Filesystem\Filesystem"></service>
+        <service id="bazinga.jstranslation.finder_factory" class="Bazinga\Bundle\JsTranslationBundle\Finder\FinderFactory"></service>


such services should be private

Agree. I am changing it.

stof · 2016-10-21T09:55:44Z

Extractor/Extractor.php

+        $this->prefix = $prefix;
+    }
+
+    protected function parseMessagesFromContent($fileContent, MessageCatalogue $catalogue)


all these should be private. There is no reason to introduce extension points needing to be maintained for BC.

True. I am changing it.

stof · 2016-10-21T09:56:01Z

Extractor/Extractor.php

+            return $matches[1];
+        }
+
+        return [];


no short array syntax

stof · 2016-10-21T09:56:50Z

Extractor/Extractor.php

+     */
+    public function extract($resource, MessageCatalogue $catalogue)
+    {
+        $assetsPath = dirname($resource) . '/public';


this looks weird to me

This extractor is meant to search for assets (which in 99% of the cases lies on the Resources/public folder). Since this checks every single file, restricting to the public folder improves the performance dramatically.

$resource will already be the path to Appbundle/Resources when Symfony calls the extractor. So using dirname() actually breaks it

jcchavezs · 2016-10-21T11:54:51Z

@stof agree with the fact that the removal of the support to PHP 5.3 is in the scope of another PR. Before changing the incompatible code I would try to talk with @willdurand about it.

stof · 2016-10-21T11:56:54Z

@jcchavezs fixes for PHP 5.3 compat were done by @monteiro (new maintainer of the bundle) a few days ago, so I guess this is not yet planned

jcchavezs · 2016-10-21T12:49:33Z

@stof I just added fixes for making it compatible with PHP 5.3 but the CI hasn't triggered yet.

jcchavezs · 2016-10-24T12:21:26Z

@monteiro can we merge this?

craigh · 2016-12-29T15:40:44Z

What's going on with this PR?

monteiro · 2017-05-02T08:14:38Z

@jcchavezs thanks a lot for this PR. Looks very good. Can you address @stof 's comments so we can move forward with this?

mcorteel · 2017-09-08T11:02:31Z

@jcchavezs Did you abandon this PR?

jcchavezs · 2017-09-11T17:48:05Z

Nope. I've been busy but I will take some time to work on this by this week.

jeremy-richard · 2018-01-29T08:07:40Z

This feature is awesome. Do you need help to release it ?

jcchavezs · 2018-01-29T09:26:45Z

@jeremy-richard hi there, honestly I did not have time to fix @stof comments but if you can do it I am more than happy to merge this.

jeremy-richard · 2018-01-29T10:38:03Z

@jcchavezs OK, I forked BazingaJsTranslationBundle and checkout your branch.
First thing first I did a rebase to be up to date with the master branch.

Can you give me a hint about what is done, what needs to be done please?

jeremy-richard · 2018-01-29T10:39:09Z

Tests/Extractor/CoffeeExtractorTest.php

+        $this->givenASourceFolder();
+
+        $this->fileContent = <<<STRING
+        Translator.tras 'test-key-1'


@jcchavezs Is there a typo? Should this be trans instaed of tras ?

There is. Sorry.

my bad in fact it is not a typo as this test NotValidTransFunctionUsage

jcchavezs · 2018-01-29T11:06:36Z

@jeremy-richard everything was working but there are three comments that must be addressed:

[#163] Added support for translation extractors for JS #164 (comment)
https://github.com/willdurand/BazingaJsTranslationBundle/pull/164/files/413c12d0a87a7da345a61ed621c2099260d9fa7f#diff-6cb71bac1e0b1d9b34177249d6f1e0db => remove the factory class and test the finder with an actual file.
[#163] Added support for translation extractors for JS #164 (comment)

cf. willdurand#164 (comment)

cf. https://github.com/willdurand/BazingaJsTranslationBundle/pull/164/files/413c12d0a87a7da345a61ed621c2099260d9fa7f#diff-6cb71bac1e0b1d9b34177249d6f1e0db willdurand#164 (comment)

jeremy-richard · 2018-01-29T13:56:05Z

OK, I pushed some commits to my branch https://github.com/jeremy-richard/BazingaJsTranslationBundle/tree/163_addition_of_js_and_coffee_extractor.

How do we proceed? Should I open a PR on @jcchavezs repository or should I open a new PR here?

jcchavezs · 2018-01-29T14:29:35Z

@jeremy-richard I'd say that you open your own PR against master so you can easily fix the comments people might have. Link this one so we keep context.

Really thanks for doing this!

jcchavezs mentioned this pull request Jun 10, 2016

Translation extractor #131

Closed

jcchavezs reviewed Jun 10, 2016
View reviewed changes

stof reviewed Jun 10, 2016
View reviewed changes

Extractor/Extractor.php

public function __construct(

Filesystem $filesystem,

Finder $finder

Copy link

Contributor

stof Jun 10, 2016

Choose a reason for hiding this comment

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

the signature should stay on 1 line

cmfcmf reviewed Aug 3, 2016
View reviewed changes

jcchavezs added 4 commits October 21, 2016 10:34

[willdurand#163] Added support for translation extractors for JS and …

d6c5f95

…Coffeescript and the inclusion of phpunit for the tests.

[willdurand#163] Fixed a bug in the declaration of extractors in Reso…

d0bcd45

…urces/config/services.xml.

[willdurand#163] Added documentation for the extractors.

30bc9f6

[willdurand#163] Fixed a bug when using the finder in order to make t…

4bd1f9d

…he extractors stateless.

jcchavezs force-pushed the 163_addition_of_js_and_coffee_extractor branch from a2fef88 to 629aa7b Compare October 21, 2016 08:36

[willdurand#163] Added the versioning for symfony/translation in orde…

413c12d

…r to not having broken inheritances.

jcchavezs force-pushed the 163_addition_of_js_and_coffee_extractor branch from 629aa7b to 413c12d Compare October 21, 2016 08:48

stof reviewed Oct 21, 2016

View reviewed changes

[willdurand#163] Fixes based on PR comments.

26a9f3c

jcchavezs added 3 commits October 21, 2016 14:14

[willdurand#163] Fixed a bug in the regex for extracting transChoice.

6f53529

[willdurand#163] Fixed compatibility with PHP 5.3.

221d20a

[willdurand#163] Made filesystem and finder_factory private.

0451839

stof mentioned this pull request Aug 4, 2017

Fix Travis configuration for different distributions #212

Merged

jeremy-richard reviewed Jan 29, 2018

View reviewed changes

jeremy-richard added a commit to jeremy-richard/BazingaJsTranslationBundle that referenced this pull request Jan 29, 2018

[willdurand#163] Fixes based on PR comment

4137824

cf. willdurand#164 (comment)

jeremy-richard mentioned this pull request Jan 29, 2018

[#163] Added support for translation extractors for JS #238

Open

jcchavezs closed this Dec 3, 2019

[#163] Added support for translation extractors for JS #164

[#163] Added support for translation extractors for JS #164

Conversation

jcchavezs commented Jun 10, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented Jun 10, 2016

Choose a reason for hiding this comment

jcchavezs commented Jun 17, 2016

cmfcmf Aug 3, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetide commented Oct 17, 2016

jcchavezs commented Oct 21, 2016 • edited

stof commented Oct 21, 2016

jcchavezs commented Oct 21, 2016

stof commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented Oct 21, 2016

stof commented Oct 21, 2016

jcchavezs commented Oct 21, 2016

jcchavezs commented Oct 24, 2016

craigh commented Dec 29, 2016

monteiro commented May 2, 2017

mcorteel commented Sep 8, 2017

jcchavezs commented Sep 11, 2017 via email • edited

jeremy-richard commented Jan 29, 2018

jcchavezs commented Jan 29, 2018

jeremy-richard commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented Jan 29, 2018 • edited

jeremy-richard commented Jan 29, 2018

jcchavezs commented Jan 29, 2018

jcchavezs commented Jun 10, 2016 •

edited

cmfcmf Aug 3, 2016 •

edited

jcchavezs commented Oct 21, 2016 •

edited

jcchavezs Oct 21, 2016 •

edited

jcchavezs Oct 21, 2016 •

edited

jcchavezs commented Sep 11, 2017 via email •

edited

jcchavezs commented Jan 29, 2018 •

edited