Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

case-insensitive for Operator name #93

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

case-insensitive for Operator name #93

wants to merge 4 commits into from

Conversation

Pierozi
Copy link
Member

@Pierozi Pierozi commented Sep 14, 2016 •
edited by Hywan
Loading

Address to #92
Fix #92.

Edit by @Hywan.

@Hywan Hywan self-assigned this Sep 14, 2016
Hywan
Hywan reviewed Sep 14, 2016
View reviewed changes
Test/Integration/Model/Operator.php Outdated
@@ -204,4 +204,30 @@ function ($b) use (&$gExecuted) {
->boolean($gExecuted)
->isTrue();
}

public function case_insensitive()
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure we can write unit test cases here. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

The reason is that we do not require an integration test case to check that Asserter::$_operators has change operator's name cases. This is a typical case of a unit test.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's true, but this case also cover the visitor rule are as expected, not only how operator are stored.
You think these three test are bit overkill?

Copy link
Member

Choose a reason for hiding this comment

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

You only need to test the asserter visitor. Why do you want to test something else?

@Pierozi
Copy link
Member Author

Pierozi commented Sep 14, 2016

@Hywan Unit Test like this ?
I will rebase when it's ok.

Hywan
Hywan reviewed Sep 14, 2016
View reviewed changes
Test/Unit/Visitor/Asserter.php Outdated
)
->when($result = $asserter->setOperator('_FOO_', $operator))
->then
->boolean($asserter->operatorExists('_foo_'))
Copy link
Member

Choose a reason for hiding this comment

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

You can also check that _FOO_ does not exist.

@Hywan
Copy link
Member

Hywan commented Sep 14, 2016 •
edited
Loading

@Pierozi It looks good modulo my comment 👍.

@Pierozi Pierozi force-pushed the fix/setOperatorInsensitive branch from 9f10a86 to e6bea82 Compare September 14, 2016 13:02
@Pierozi
Copy link
Member Author

Pierozi commented Sep 14, 2016

Done 👍

@Hywan Hywan assigned jubianchi and unassigned Hywan Sep 14, 2016
@Hywan
Copy link
Member

Hywan commented Sep 14, 2016

I am assigning @jubianchi for the review!

@Pierozi
Copy link
Member Author

Pierozi commented Jan 11, 2017

Quick review ?

Hywan
Hywan suggested changes Jan 11, 2017
View reviewed changes
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

The getter should ony change the case.
Also, in the Operator constructor, we must change the case there, instead of when instanciating the Operator class.

Test/Unit/Visitor/Asserter.php
{
$this
->given(
$asserter = new SUT(),
Copy link
Member

Choose a reason for hiding this comment

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

Please, run hoa devtools:cs, and simply align =.

Test/Unit/Visitor/Asserter.php
@@ -418,6 +418,23 @@ public function case_set_operator()
->isEqualTo(xcallable($operator));
}

public function case_set_operator_insensitive()
Copy link
Member

Choose a reason for hiding this comment

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

Please, add a case with a non-ASCII symbol, like Λ and λ (resp the capital and small variant). Is it what we want? I guess yes.

@Pierozi Pierozi assigned Pierozi and unassigned jubianchi Mar 16, 2017
@Pierozi Pierozi requested a review from jubianchi March 16, 2017 23:01
@Pierozi Pierozi force-pushed the fix/setOperatorInsensitive branch from 4ca678a to 388e75e Compare March 16, 2017 23:07
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.132% when pulling 388e75e on Pierozi:fix/setOperatorInsensitive into c9a965c on hoaproject:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.132% when pulling 388e75e on Pierozi:fix/setOperatorInsensitive into c9a965c on hoaproject:master.

@coveralls
Copy link

coveralls commented Mar 16, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 96.132% when pulling 388e75e on Pierozi:fix/setOperatorInsensitive into c9a965c on hoaproject:master.

@Pierozi
Copy link
Member Author

Pierozi commented Mar 16, 2017

It seems casting of Extended ASCII work only from php >= 5.6 as Travis build report.

@Hywan
Copy link
Member

Hywan commented Apr 6, 2017

@Pierozi In this case, I propose to wait on PHP 7 to land in Hoa to merge this PR. It will come really soon. Agree?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Hywan Hywan Hywan requested changes

@jubianchi jubianchi Awaiting requested review from jubianchi

Assignees

@Pierozi Pierozi

Labels
difficulty: medium enhancement in progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Pierozi @Hywan @coveralls @jubianchi