Uh oh!
There was an error while loading. Please reload this page.
Enhance docs of Statement constructor and add alternate constructors - #205
Enhance docs of Statement constructor and add alternate constructors#205JeroenDeDauw wants to merge 1 commit into
Conversation
JeroenDeDauw
commented
Sep 25, 2014
@Benestar this is a follow up to the work you did. I've found that there are many places where you want to construct a |
JeroenDeDauw
commented
Sep 25, 2014
@Benestar Your changes to the constructor are also not really reflected in things such as |
There was a problem hiding this comment.
This method has the same signature as the constructure and is not helpful in my opinion.
There was a problem hiding this comment.
This is a named constructor. Also, one can use this now, without getting any IDE warnings due to parameter mismatch.
Benestar
commented
Sep 25, 2014
Yeah, I think moving this into a static method is the right way.
I'm totally fine with keeping those shortcut methods as they are. However, might there also be situations where you have a |
There was a problem hiding this comment.
This syntax means that all parameters are optional now. But the first should not be optional, as it was before.
There was a problem hiding this comment.
The first parameter is not necessarily a claim. So putting that in there is deceptive.
There was a problem hiding this comment.
So? $snakOrClaim. The first parameter was and still is required, right? Making it optional does not help.
There was a problem hiding this comment.
It is not optional. Though an explicit check for it would indeed be good
6faeb9b to
0013a9dCompareBenestar
commented
Oct 11, 2014
Since the next release is 2.0 anyways we could also break the constructor signature as it is quite easy to adopt the new form. |
JeroenDeDauw
commented
Oct 11, 2014
@Benestar if someone can already make the needed changes in WB.git, then let's do that yes. |
No description provided.