Skip to content

Conversation

mikeselander
Copy link
Contributor

@mikeselander mikeselander commented Mar 23, 2019

In WPCS 1.1.0, the allowMultipleArguments property from PEAR.Functions.FunctionCallSignature was disallowed, causing our tests to fail and existing behavior to be altered.

I re-allowed this to restore expected behavior and updated Composer to more explicitly show which version we are on (^1.0.0 was already giving us 1.2.1, just without the explicit declaration).

We can easily turn this back off if we decide to alter the behavior in the future. I believe this to be the only "breaking"/expectation-changing behavior from WPCS 1.0.0 -> 1.2.0.

Copy link
Contributor

@kadamwhite kadamwhite left a comment

Choose a reason for hiding this comment

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

I think conceptually I'm in favor of the change they made, and aside from the tests for this repository a quick smoke-test of HM project code indicates we follow this rule pretty consistently. That said, +1 on rolling it back locally until we can make that switch more mindfully.

@mikeselander
Copy link
Contributor Author

Agreed, I don't have a problem with their philosophy. I'm more afraid of this throwing a bunch of new flags all of a sudden for developers without greater discussion of the topic. I'll open an Issues for this.

rmccue
rmccue previously requested changes Mar 26, 2019
@mikeselander
Copy link
Contributor Author

@rmccue I've adjusted the spaces - any more thoughts on this change?

@mikeselander mikeselander added this to the 0.6 milestone Mar 29, 2019
@mikeselander mikeselander dismissed rmccue’s stale review March 29, 2019 18:23

Feedback addressed.

@mikeselander mikeselander merged commit 60b4af5 into master Mar 29, 2019
@mikeselander mikeselander deleted the fix-multiple-arguments branch March 29, 2019 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants