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

CLDR specification entities #8581

Merged
merged 13 commits into from Jan 3, 2018

Conversation

Projects
None yet
4 participants
@LittleBigDev
Contributor

LittleBigDev commented Dec 5, 2017

Questions Answers
Branch? develop
Description? In order to format numbers and currencies, Locale needs two entities:
- NumberSpecification - Used to format numbers
- CurrencySpecification - Used to format amounts
CurrencySpecification extends NumberSpecification
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? FF-137
How to test? See Jira issue

This change is Reviewable

@LittleBigDev LittleBigDev added the WIP label Dec 5, 2017

@LittleBigDev LittleBigDev changed the title from WIP : pushing for checkout purpose to WIP : CLDR specification entities Dec 5, 2017

@mickaelandrieu mickaelandrieu added the 1.7.x label Dec 5, 2017

@LittleBigDev LittleBigDev changed the title from WIP : CLDR specification entities to CLDR specification entities Dec 6, 2017

@LittleBigDev LittleBigDev removed the WIP label Dec 6, 2017

@LittleBigDev LittleBigDev referenced this pull request Dec 8, 2017

Merged

CLDR number formatter #8597

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Dec 8, 2017

Contributor

Needs unit tests.

Contributor

LittleBigDev commented Dec 8, 2017

Needs unit tests.

@LittleBigDev LittleBigDev added the WIP label Dec 8, 2017

@LittleBigDev LittleBigDev removed the WIP label Dec 11, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Dec 11, 2017

Member

Reviewed 2 of 7 files at r1, 8 of 8 files at r3.
Review status: all files reviewed at latest revision, 14 unresolved discussions.


src/PrestaShopBundle/Localization/Exception/Exception.php, line 29 at r3 (raw file):

namespace PrestaShopBundle\Localization\Exception;

class Exception extends \Exception

While this is correct, I think that "LocalizationException" would be better.

I find that using simply "Exception" has two disadvantages:

  1. When you read "throw Exception" in the code, you cannot know right away which kind of exception is being thrown.
  2. When you start typing "throw Exc|" you'll have dozens of Exception on your autocomplete list.

src/PrestaShopBundle/Localization/Exception/InvalidArgumentException.php, line 29 at r3 (raw file):

namespace PrestaShopBundle\Localization\Exception;

class InvalidArgumentException extends \InvalidArgumentException
  1. Unless you are targeting to catch a specific type of exception, don't over specify.
  2. This exception does not extend Localization\Exception\Exception, which means that if you want to catch all exceptions thrown by Localization you need two separate catch.

The way I see it, typed exceptions should be used ONLY for targeted catching. If you want to send information, you should use the exception message.


src/PrestaShopBundle/Localization/Specification/Number.php, line 64 at r3 (raw file):


    /**
     * @var NumberSymbolList[]

Missing description. You should say that this is indexed by numbering system


src/PrestaShopBundle/Localization/Specification/Number.php, line 116 at r3 (raw file):

     *  The symbols list to use when formatting in this numbering system
     */
    public function addSymbols($numberingSystem, NumberSymbolList $symbolList)

Why not set everything in the constructor?
In which use case this class should be mutated?


src/PrestaShopBundle/Localization/Specification/Number.php, line 132 at r3 (raw file):

     *  The fallback symbols list
     */
    public function hydrateSymbols($numberingSystem, NumberSymbolList $fallbackSymbolList)

Why is this public?


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 30 at r3 (raw file):


/**
 * Class NumberSymbolList

Redundant information


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 173 at r3 (raw file):

     *   Fluent interface
     */
    public function hydrate(NumberSymbolList $defaultList)

Why not set in the constructor?


src/PrestaShopBundle/Localization/Specification/Percentage.php, line 37 at r3 (raw file):

 * system (latin, arab, ...).
 */
class Percentage extends NumberSpecification

Is this a placeholder or is it supposed to be the same as NumberSpecification?


src/PrestaShopBundle/Tests/Localization/Specification/CurrencyTest.php, line 40 at r3 (raw file):

    {
        $this->numberSpec = new Currency();
    }

There are no tests in this file


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 53 at r3 (raw file):

     * (also tests addSymbols() at the same time)
     */
    public function testGetAllSymbols()

Try to name the test the same as the thing that you are asserting, so that makes a logical phrase. For example: "testGetAllSymbolsReturnsAListOfSymbols" -> "test get all symbols returns a list of symbols"


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 61 at r3 (raw file):


        $this->numberSpec->addSymbols('latin', $latinList);
        $this->numberSpec->addSymbols('arab', $arabList);

I don't think specifications should be mutable


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 68 at r3 (raw file):

                'arab'  => $arabList,
            ],
            $this->numberSpec->getAllSymbols()

Is there a use case for getting all symbols at once?


src/PrestaShopBundle/Tests/Localization/Specification/PercentageTest.php, line 40 at r3 (raw file):

    {
        $this->numberSpec = new Percentage();
    }

There are no tests in this file


tests/sf-tests.xml, line 26 at r3 (raw file):

    </testsuite>
    <testsuite name="CLDR Test Suite">
      <directory>../src/PrestaShopBundle/Tests</directory>

Actually it looks like this directory should replace the directory of "PrestaShop Bundle Test Suite", shouldn't it?


Comments from Reviewable

Member

eternoendless commented Dec 11, 2017

Reviewed 2 of 7 files at r1, 8 of 8 files at r3.
Review status: all files reviewed at latest revision, 14 unresolved discussions.


src/PrestaShopBundle/Localization/Exception/Exception.php, line 29 at r3 (raw file):

namespace PrestaShopBundle\Localization\Exception;

class Exception extends \Exception

While this is correct, I think that "LocalizationException" would be better.

I find that using simply "Exception" has two disadvantages:

  1. When you read "throw Exception" in the code, you cannot know right away which kind of exception is being thrown.
  2. When you start typing "throw Exc|" you'll have dozens of Exception on your autocomplete list.

src/PrestaShopBundle/Localization/Exception/InvalidArgumentException.php, line 29 at r3 (raw file):

namespace PrestaShopBundle\Localization\Exception;

class InvalidArgumentException extends \InvalidArgumentException
  1. Unless you are targeting to catch a specific type of exception, don't over specify.
  2. This exception does not extend Localization\Exception\Exception, which means that if you want to catch all exceptions thrown by Localization you need two separate catch.

The way I see it, typed exceptions should be used ONLY for targeted catching. If you want to send information, you should use the exception message.


src/PrestaShopBundle/Localization/Specification/Number.php, line 64 at r3 (raw file):


    /**
     * @var NumberSymbolList[]

Missing description. You should say that this is indexed by numbering system


src/PrestaShopBundle/Localization/Specification/Number.php, line 116 at r3 (raw file):

     *  The symbols list to use when formatting in this numbering system
     */
    public function addSymbols($numberingSystem, NumberSymbolList $symbolList)

Why not set everything in the constructor?
In which use case this class should be mutated?


src/PrestaShopBundle/Localization/Specification/Number.php, line 132 at r3 (raw file):

     *  The fallback symbols list
     */
    public function hydrateSymbols($numberingSystem, NumberSymbolList $fallbackSymbolList)

Why is this public?


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 30 at r3 (raw file):


/**
 * Class NumberSymbolList

Redundant information


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 173 at r3 (raw file):

     *   Fluent interface
     */
    public function hydrate(NumberSymbolList $defaultList)

Why not set in the constructor?


src/PrestaShopBundle/Localization/Specification/Percentage.php, line 37 at r3 (raw file):

 * system (latin, arab, ...).
 */
class Percentage extends NumberSpecification

Is this a placeholder or is it supposed to be the same as NumberSpecification?


src/PrestaShopBundle/Tests/Localization/Specification/CurrencyTest.php, line 40 at r3 (raw file):

    {
        $this->numberSpec = new Currency();
    }

There are no tests in this file


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 53 at r3 (raw file):

     * (also tests addSymbols() at the same time)
     */
    public function testGetAllSymbols()

Try to name the test the same as the thing that you are asserting, so that makes a logical phrase. For example: "testGetAllSymbolsReturnsAListOfSymbols" -> "test get all symbols returns a list of symbols"


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 61 at r3 (raw file):


        $this->numberSpec->addSymbols('latin', $latinList);
        $this->numberSpec->addSymbols('arab', $arabList);

I don't think specifications should be mutable


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 68 at r3 (raw file):

                'arab'  => $arabList,
            ],
            $this->numberSpec->getAllSymbols()

Is there a use case for getting all symbols at once?


src/PrestaShopBundle/Tests/Localization/Specification/PercentageTest.php, line 40 at r3 (raw file):

    {
        $this->numberSpec = new Percentage();
    }

There are no tests in this file


tests/sf-tests.xml, line 26 at r3 (raw file):

    </testsuite>
    <testsuite name="CLDR Test Suite">
      <directory>../src/PrestaShopBundle/Tests</directory>

Actually it looks like this directory should replace the directory of "PrestaShop Bundle Test Suite", shouldn't it?


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Dec 11, 2017

Contributor

Review status: all files reviewed at latest revision, 14 unresolved discussions.


src/PrestaShopBundle/Localization/Exception/Exception.php, line 29 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

While this is correct, I think that "LocalizationException" would be better.

I find that using simply "Exception" has two disadvantages:

  1. When you read "throw Exception" in the code, you cannot know right away which kind of exception is being thrown.
  2. When you start typing "throw Exc|" you'll have dozens of Exception on your autocomplete list.

Done.


src/PrestaShopBundle/Localization/Exception/InvalidArgumentException.php, line 29 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…
  1. Unless you are targeting to catch a specific type of exception, don't over specify.
  2. This exception does not extend Localization\Exception\Exception, which means that if you want to catch all exceptions thrown by Localization you need two separate catch.

The way I see it, typed exceptions should be used ONLY for targeted catching. If you want to send information, you should use the exception message.

I'll simplify exceptions usage


src/PrestaShopBundle/Localization/Specification/Number.php, line 64 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing description. You should say that this is indexed by numbering system

Done.


src/PrestaShopBundle/Localization/Specification/Number.php, line 116 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not set everything in the constructor?
In which use case this class should be mutated?

Number specification is built aggregating data from multiple sources. I think using yet another data object dedicated to aggregation would be overkill.
So, I implemented a way to extend / hydrate missing data inside this class (and also inside NumberSymbolList class, for the same reason).


src/PrestaShopBundle/Localization/Specification/Number.php, line 132 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why is this public?

The factory / builder / manager class in charge for building the spec object will need to inject data for extension / hydration.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 173 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not set in the constructor?

Same reason as for Number spec class


src/PrestaShopBundle/Localization/Specification/Percentage.php, line 37 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Is this a placeholder or is it supposed to be the same as NumberSpecification?

For business reason, I thought having a specific name for percentage spec was a good idea. But it works exactly the same. I didn't see anything different from vanilla number spec (only the number patterns are different because they contain an additional character : "%")


src/PrestaShopBundle/Tests/Localization/Specification/CurrencyTest.php, line 40 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

There are no tests in this file

As CurrencyTest extends NumberTest, the tests are ran again but this time with a Currency spec object. I want those tests to be green with any Number specification class.


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 61 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I don't think specifications should be mutable

@see comments above addressing the same subject


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 68 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Is there a use case for getting all symbols at once?

Sometimes when writing in a given language and expecting a specific numbering system, this numbering system is not available in CLDR data. I wanted to make sure there is a way to list all available symbol lists and pick one of them.
Though, this is very unlikely.... I think removing the method would be harmless. Your call.


src/PrestaShopBundle/Tests/Localization/Specification/PercentageTest.php, line 40 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

There are no tests in this file

See comments about Currency spec tests.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Actually it looks like this directory should replace the directory of "PrestaShop Bundle Test Suite", shouldn't it?

I agree. But I didn't dare touching it. We could give it a try, but is this the good moment to do it ?


Comments from Reviewable

Contributor

LittleBigDev commented Dec 11, 2017

Review status: all files reviewed at latest revision, 14 unresolved discussions.


src/PrestaShopBundle/Localization/Exception/Exception.php, line 29 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

While this is correct, I think that "LocalizationException" would be better.

I find that using simply "Exception" has two disadvantages:

  1. When you read "throw Exception" in the code, you cannot know right away which kind of exception is being thrown.
  2. When you start typing "throw Exc|" you'll have dozens of Exception on your autocomplete list.

Done.


src/PrestaShopBundle/Localization/Exception/InvalidArgumentException.php, line 29 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…
  1. Unless you are targeting to catch a specific type of exception, don't over specify.
  2. This exception does not extend Localization\Exception\Exception, which means that if you want to catch all exceptions thrown by Localization you need two separate catch.

The way I see it, typed exceptions should be used ONLY for targeted catching. If you want to send information, you should use the exception message.

I'll simplify exceptions usage


src/PrestaShopBundle/Localization/Specification/Number.php, line 64 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing description. You should say that this is indexed by numbering system

Done.


src/PrestaShopBundle/Localization/Specification/Number.php, line 116 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not set everything in the constructor?
In which use case this class should be mutated?

Number specification is built aggregating data from multiple sources. I think using yet another data object dedicated to aggregation would be overkill.
So, I implemented a way to extend / hydrate missing data inside this class (and also inside NumberSymbolList class, for the same reason).


src/PrestaShopBundle/Localization/Specification/Number.php, line 132 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why is this public?

The factory / builder / manager class in charge for building the spec object will need to inject data for extension / hydration.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 173 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not set in the constructor?

Same reason as for Number spec class


src/PrestaShopBundle/Localization/Specification/Percentage.php, line 37 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Is this a placeholder or is it supposed to be the same as NumberSpecification?

For business reason, I thought having a specific name for percentage spec was a good idea. But it works exactly the same. I didn't see anything different from vanilla number spec (only the number patterns are different because they contain an additional character : "%")


src/PrestaShopBundle/Tests/Localization/Specification/CurrencyTest.php, line 40 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

There are no tests in this file

As CurrencyTest extends NumberTest, the tests are ran again but this time with a Currency spec object. I want those tests to be green with any Number specification class.


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 61 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I don't think specifications should be mutable

@see comments above addressing the same subject


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 68 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Is there a use case for getting all symbols at once?

Sometimes when writing in a given language and expecting a specific numbering system, this numbering system is not available in CLDR data. I wanted to make sure there is a way to list all available symbol lists and pick one of them.
Though, this is very unlikely.... I think removing the method would be harmless. Your call.


src/PrestaShopBundle/Tests/Localization/Specification/PercentageTest.php, line 40 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

There are no tests in this file

See comments about Currency spec tests.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Actually it looks like this directory should replace the directory of "PrestaShop Bundle Test Suite", shouldn't it?

I agree. But I didn't dare touching it. We could give it a try, but is this the good moment to do it ?


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Dec 11, 2017

Contributor

Review status: 5 of 9 files reviewed at latest revision, 14 unresolved discussions.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 30 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Redundant information

Done.


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 53 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Try to name the test the same as the thing that you are asserting, so that makes a logical phrase. For example: "testGetAllSymbolsReturnsAListOfSymbols" -> "test get all symbols returns a list of symbols"

Done.


Comments from Reviewable

Contributor

LittleBigDev commented Dec 11, 2017

Review status: 5 of 9 files reviewed at latest revision, 14 unresolved discussions.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 30 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Redundant information

Done.


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 53 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Try to name the test the same as the thing that you are asserting, so that makes a logical phrase. For example: "testGetAllSymbolsReturnsAListOfSymbols" -> "test get all symbols returns a list of symbols"

Done.


Comments from Reviewable

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 12, 2017

@eternoendless eternoendless added this to the 1.7.4.0 milestone Dec 13, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Dec 13, 2017

Member

Reviewed 2 of 6 files at r4, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


src/PrestaShopBundle/Localization/Specification/Currency.php, line 58 at r5 (raw file):

    protected $currencyDisplay;

    public function __construct(

Missing parameters description


src/PrestaShopBundle/Localization/Specification/Currency.php, line 68 at r5 (raw file):

        $secondaryGroupSize = null,
        $currencyDisplay = null
    ) {

You shouldn't use defaults on mandatory fields, otherwise you make it possible to create an incomplete entity


src/PrestaShopBundle/Localization/Specification/Number.php, line 39 at r5 (raw file):

class Number
{
    public function __construct(

Missing parameters description


src/PrestaShopBundle/Localization/Specification/Number.php, line 48 at r5 (raw file):

        $primaryGroupSize = null,
        $secondaryGroupSize = null
    ) {

Same comment here, don't use default values on mandatory parameters


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 149 at r5 (raw file):

    protected $currencyGroup;

    public function __construct(

Missing parameters description


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 161 at r5 (raw file):

        $infinity = null,
        $nan = null
    ) {

Same comment about mandatory parameters


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 68 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Sometimes when writing in a given language and expecting a specific numbering system, this numbering system is not available in CLDR data. I wanted to make sure there is a way to list all available symbol lists and pick one of them.
Though, this is very unlikely.... I think removing the method would be harmless. Your call.

I guess we can keep it


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I agree. But I didn't dare touching it. We could give it a try, but is this the good moment to do it ?

Please try in a separate PR and eventually rebase this one


Comments from Reviewable

Member

eternoendless commented Dec 13, 2017

Reviewed 2 of 6 files at r4, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


src/PrestaShopBundle/Localization/Specification/Currency.php, line 58 at r5 (raw file):

    protected $currencyDisplay;

    public function __construct(

Missing parameters description


src/PrestaShopBundle/Localization/Specification/Currency.php, line 68 at r5 (raw file):

        $secondaryGroupSize = null,
        $currencyDisplay = null
    ) {

You shouldn't use defaults on mandatory fields, otherwise you make it possible to create an incomplete entity


src/PrestaShopBundle/Localization/Specification/Number.php, line 39 at r5 (raw file):

class Number
{
    public function __construct(

Missing parameters description


src/PrestaShopBundle/Localization/Specification/Number.php, line 48 at r5 (raw file):

        $primaryGroupSize = null,
        $secondaryGroupSize = null
    ) {

Same comment here, don't use default values on mandatory parameters


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 149 at r5 (raw file):

    protected $currencyGroup;

    public function __construct(

Missing parameters description


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 161 at r5 (raw file):

        $infinity = null,
        $nan = null
    ) {

Same comment about mandatory parameters


src/PrestaShopBundle/Tests/Localization/Specification/NumberTest.php, line 68 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Sometimes when writing in a given language and expecting a specific numbering system, this numbering system is not available in CLDR data. I wanted to make sure there is a way to list all available symbol lists and pick one of them.
Though, this is very unlikely.... I think removing the method would be harmless. Your call.

I guess we can keep it


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I agree. But I didn't dare touching it. We could give it a try, but is this the good moment to do it ?

Please try in a separate PR and eventually rebase this one


Comments from Reviewable

@eternoendless

Some changes required

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Dec 13, 2017

Contributor

Here are my changes


Review status: 3 of 9 files reviewed at latest revision, 7 unresolved discussions.


src/PrestaShopBundle/Localization/Specification/Currency.php, line 58 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing parameters description

Done.


src/PrestaShopBundle/Localization/Specification/Currency.php, line 68 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You shouldn't use defaults on mandatory fields, otherwise you make it possible to create an incomplete entity

Done.


src/PrestaShopBundle/Localization/Specification/Number.php, line 39 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing parameters description

Done.


src/PrestaShopBundle/Localization/Specification/Number.php, line 48 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Same comment here, don't use default values on mandatory parameters

Done.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 149 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing parameters description

Done.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 161 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Same comment about mandatory parameters

Done.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Please try in a separate PR and eventually rebase this one

I tried and faced a lot of autoloading issues. I think we should think about moving the old "PrestaShop Bundle Test Suite" only for branches depending on the current develop branch (containing sf files migration, new files structure etc)


Comments from Reviewable

Contributor

LittleBigDev commented Dec 13, 2017

Here are my changes


Review status: 3 of 9 files reviewed at latest revision, 7 unresolved discussions.


src/PrestaShopBundle/Localization/Specification/Currency.php, line 58 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing parameters description

Done.


src/PrestaShopBundle/Localization/Specification/Currency.php, line 68 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You shouldn't use defaults on mandatory fields, otherwise you make it possible to create an incomplete entity

Done.


src/PrestaShopBundle/Localization/Specification/Number.php, line 39 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing parameters description

Done.


src/PrestaShopBundle/Localization/Specification/Number.php, line 48 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Same comment here, don't use default values on mandatory parameters

Done.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 149 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing parameters description

Done.


src/PrestaShopBundle/Localization/Specification/NumberSymbolList.php, line 161 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Same comment about mandatory parameters

Done.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Please try in a separate PR and eventually rebase this one

I tried and faced a lot of autoloading issues. I think we should think about moving the old "PrestaShop Bundle Test Suite" only for branches depending on the current develop branch (containing sf files migration, new files structure etc)


Comments from Reviewable

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Dec 13, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 5
           

See the complete overview on Codacy

codacy-bot commented Dec 13, 2017

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 5
           

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Dec 13, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Dec 14, 2017

Member

Reviewed 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I tried and faced a lot of autoloading issues. I think we should think about moving the old "PrestaShop Bundle Test Suite" only for branches depending on the current develop branch (containing sf files migration, new files structure etc)

I'd prefer not to merge this until it's changed and this PR is rebased


Comments from Reviewable

Member

eternoendless commented Dec 14, 2017

Reviewed 6 of 6 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

I tried and faced a lot of autoloading issues. I think we should think about moving the old "PrestaShop Bundle Test Suite" only for branches depending on the current develop branch (containing sf files migration, new files structure etc)

I'd prefer not to merge this until it's changed and this PR is rebased


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Dec 14, 2017

Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I'd prefer not to merge this until it's changed and this PR is rebased

Ok, incomming.


Comments from Reviewable

Contributor

LittleBigDev commented Dec 14, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I'd prefer not to merge this until it's changed and this PR is rebased

Ok, incomming.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Jan 3, 2018

Member

Reviewed 6 of 6 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Ok, incomming.

You forgot to delete this part which is no longer valid


Comments from Reviewable

Member

eternoendless commented Jan 3, 2018

Reviewed 6 of 6 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, LittleBigDev (Bastien Bieri) wrote…

Ok, incomming.

You forgot to delete this part which is no longer valid


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Jan 3, 2018

Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You forgot to delete this part which is no longer valid

Done.


Comments from Reviewable

Contributor

LittleBigDev commented Jan 3, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


tests/sf-tests.xml, line 26 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You forgot to delete this part which is no longer valid

Done.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Jan 3, 2018

Member

:lgtm:


Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Jan 3, 2018

:lgtm:


Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Jan 3, 2018

Thank you @LittleBigDev

@eternoendless eternoendless merged commit 006eab4 into PrestaShop:develop Jan 3, 2018

2 of 3 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
code-review/reviewable 9 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment