search

hoaproject / Ruler

The Hoa\Ruler library.
https://hoa-project.net
627 stars 66 forks source link

Disassembly: Always add parenthesis around operators #81

Closed Hywan closed 8 years ago

Hywan commented 8 years ago

Fix #79. Fix #80.

Thoughts @stephpy, @Metalaka, @osaris and @shouze.

osaris commented 8 years ago

Hi @Hywan I'd like to test your PR locally but when I composer install I have the following errors :

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - atoum/praspel-extension 0.16.01.11 requires hoa/math ~0.0 -> satisfiable by hoa/math[0.14.09.16, 0.14.09.17, 0.14.09.23, 0.14.11.09, 0.14.12.10, 0.14.12.22, 0.15.02.23, 0.15.05.29, 0.15.08.25, 0.15.10.26].
    - hoa/test 2.16.01.11 requires hoa/file ~0.0 -> no matching package found.
    - hoa/math 0.15.10.26 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.15.08.25 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.15.05.29 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.15.02.23 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.14.12.22 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.14.12.10 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.14.11.09 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.14.09.23 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.14.09.17 requires hoa/compiler ~2.0 -> no matching package found.
    - hoa/math 0.14.09.16 requires hoa/compiler ~1.0 -> no matching package found.
    - hoa/test 2.16.01.14 requires atoum/praspel-extension ~0.16 -> satisfiable by atoum/praspel-extension[0.16.01.11].
    - Installation request for hoa/test ~2.0 -> satisfiable by hoa/test[2.16.01.11, 2.16.01.14].
Metalaka commented 8 years ago

@osaris composer install works with me, but I didn't test this PR changes…

$ composer require hoa/ruler

Using version 2.16.01.15 for hoa/ruler
./composer.json has been created
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Installing hoa/exception (1.16.01.11)
    Downloading: 100%

  - Installing hoa/consistency (1.16.01.14)
    Downloading: 100%

  - Installing hoa/event (1.16.01.11)
    Downloading: 100%

  - Installing hoa/visitor (2.16.01.11)
    Downloading: 100%

  - Installing hoa/iterator (2.16.01.11)
    Downloading: 100%

  - Installing hoa/protocol (1.16.01.11)
    Downloading: 100%

  - Installing hoa/stream (1.16.01.14)
    Downloading: 100%

  - Installing hoa/file (1.16.01.15)
    Downloading: 100%

  - Installing hoa/ustring (4.16.01.11)
    Downloading: 100%

  - Installing hoa/zformat (1.16.01.14)
    Downloading: 100%

  - Installing hoa/compiler (3.16.01.14)
    Downloading: 100%

  - Installing hoa/math (1.16.01.15)
    Downloading: 100%

  - Installing hoa/regex (1.16.01.15)
    Downloading: 100%

  - Installing hoa/ruler (2.16.01.15)
    Downloading: 100%

Writing lock file
Generating autoload files
osaris commented 8 years ago

Installing with --no-dev is ok. I have opened a ticket hoaproject/Test#58 on Hoa\Test because I can reproduce the bug on another lib (Compiler for example)

osaris commented 8 years ago

I confirm that this PR fix the bug in #79 I can't run tests for the moment (see my previous comment).

Do we need a test case for this ?

Hywan commented 8 years ago

We can't install hoa/test because of an atoum extension. PLease, see https://github.com/atoum/ruler-extension/pull/29 and https://github.com/atoum/ruler-extension/pull/30.

Hywan commented 8 years ago

@osaris Yup, should be great to add a test case…

jubianchi commented 8 years ago

everything should be ready for you to fix this issue. See:

It was a long journey, but we made it ;)

Hywan commented 8 years ago

Hoa\Test has been fixed. New snapshot deployed.

Hywan commented 8 years ago

ping @osaris?

julban commented 8 years ago

Hi @Hywan and @osaris , is there anything missing to get this PR merged? Thx

Hywan commented 8 years ago

@julban I don't think so. @osaris needs to review it before the merge.

osaris commented 8 years ago

@julban thanks for the reminder. Tests are green, code looks good to me. We miss a test for this bug to avoid regression, do we write it before we merge (ping @Hywan) ?

Hywan commented 8 years ago

@osaris Yup, we should. This issue https://github.com/hoaproject/Regex/issues/14 is blocking in order to get automatic test data generation unfortunately… but I think we will write manual tests before merging this patch. I don't want to introduce any regressions.

Hywan commented 8 years ago

@osaris Tests have been added. Solving the issue with Hoa\Regex is too hard for now. Ready to be merged.