zendframework / zend-inputfilter

InputFilter component from Zend Framework
BSD 3-Clause "New" or "Revised" License
64 stars 50 forks source link

Remove redundant code #17

Closed Maks3w closed 9 years ago

Maks3w commented 9 years ago

This code is the same logic present in "case 'allow_empty'" when loop cycle match

FactoryTest::testFactoryWillCreateInputWithSuggestedRequiredFlagAndAlternativeAllowEmptyFlag is the test for this case.

Maks3w commented 9 years ago

Coverage drop is fine. When there are less lines to cover then uncover lines gain weight.

Martin-P commented 9 years ago

This code is the same logic present in "case 'allow_empty'" when loop cycle match

When it loops, but what if it does not loop and only the required key is provided as an option and allow_empty is omitted? This definatelly looks like a BC break to me.

BTW the label bug is a bit surprising, because redundant code does not break anything and therefore does not represent a bug. The BC break label however seems applicable here.

Maks3w commented 9 years ago

@Martin-P There is no BC Break.

Martin-P commented 9 years ago

@Martin-P There is no BC Break.

In that case what will happen before your change and what will happen after your change in the situation I explained?

Please be more specific in your answer. Merely stating there is no BC break without any argumentation is a bit easy, isn't it? :wink:

Maks3w commented 9 years ago

@Martin-P

  1. PR description tell you what unit test cover the expected behavior
  2. I thought you know how the foreach works and what isset return. May you are confused and think in "! isset" but there is no exclamation like the one exists in the allow empty case.
Martin-P commented 9 years ago

Sorry, I was confused with another situation where required and allow_empty were treated as each other's opposites (in Zend\Validator I believe).

The unittest however only seem to cover the situation where required and allow_empty are given. A unittest for only required is missing?

Maks3w commented 9 years ago

I'm working in a deep refactor of the unit tests in another branch.