search

JeroenDeDauw / ParamProcessor

Declarative parameter processing library
https://entropywins.wtf
Other
20 stars 4 forks source link

Allow '=' in positional parameters #33

Open s7eph4n opened 6 years ago

s7eph4n commented 6 years ago

If a positional parameter contains a =, it will be split and erroneously recorded as named parameter. Example for SMW: {{#info: <span class="warning">Foobar!</span>}}

This PR introduces an additional check to make sure that the assumed named parameter is actually defined and if not, record the parameter as positional.

JeroenDeDauw commented 6 years ago

Looks good! Could you add a test for this behavior? ie one that fails if you revert the production code change but passes after its application?

s7eph4n commented 6 years ago

Will do, but I'll need a few more days.

s7eph4n commented 5 years ago

@JeroenDeDauw it took a few months instead of days, but I think I got it now.

Good thing I wrote those tests, too, because the first approach was simple, but wrong. As usual. :roll_eyes:

JeroenDeDauw commented 5 years ago

This test is quite difficult to understand. There seems to be no good reason to use a dataProvider. Having conditionals and loops in tests is typically an anti-pattern. You can likely avoid this here by having simple dedicated tests. If you run into duplication try extracting functions.

s7eph4n commented 5 years ago

Testing of ParamProcessor::setFunctionParams was completely missing, so I added it. Using a data provider seems the easiest way to reuse the existing test cases. The loop I added is needed to transform the parameters for ParamProcessor::setParameters into a parameter set suitable for ParamProcessor::setFunctionParams.

JeroenDeDauw commented 5 years ago

I am scared to merge this as-is. Maybe I get to writing better tests for this at some point