Skip to content

Added extra filter/map methods to StatementList - #285

Merged
mariushoch merged 5 commits into
masterfrom
getMainSnaks
Nov 17, 2014
Merged

Added extra filter/map methods to StatementList#285
mariushoch merged 5 commits into
masterfrom
getMainSnaks

Conversation

@JeroenDeDauw

@JeroenDeDauwJeroenDeDauw commented Nov 17, 2014

Copy link
Copy Markdown
Contributor

No description provided.

@JeroenDeDauwJeroenDeDauw changed the title Add StatementList::getMainSnaksAdded extra filter methods to StatementListNov 17, 2014
@JeroenDeDauwJeroenDeDauw changed the title Added extra filter methods to StatementListAdded extra filter/map methods to StatementListNov 17, 2014
thiemowmde
thiemowmde reviewed Nov 17, 2014
View reviewed changes
Comment threadsrc/Statement/Statement.php Outdated

@thiemowmdethiemowmdeNov 17, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not needed if #268 is merged. Also a duplicate of #265.

@JeroenDeDauwJeroenDeDauwNov 17, 2014

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ugh. This is actually not needed here. Now how do I get rid of it :|

Note how this method provides a way to construct a new Statement from a main snak in a manner neutral to the disagreement we have about the Statement constructor. It works in both cases.

@thiemowmde

thiemowmde commented Nov 17, 2014

Copy link
Copy Markdown
Contributor

Ok with the two new methods. Simple enough. Not sure why #265 is resubmitted here. Can you remove this bit?

@JeroenDeDauw

JeroenDeDauw commented Nov 17, 2014

Copy link
Copy Markdown
ContributorAuthor

Done

@JeroenDeDauw

JeroenDeDauw commented Nov 17, 2014

Copy link
Copy Markdown
ContributorAuthor

Ping @thiemowmde@adrianlang@Benestar

mariushoch
mariushoch reviewed Nov 17, 2014
View reviewed changes
Comment threadsrc/Statement/StatementList.php

@mariushochmariushochNov 17, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is overwriting the current statements, which is probably okay given the current usage of the function (in this class)... but it looks a bit odd given the function name.

@JeroenDeDauwJeroenDeDauwNov 17, 2014

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good point. There was little reason for the method anyway, so inlined now.

mariushoch added a commit that referenced this pull request Nov 17, 2014
@mariushoch
Merge pull request#285from wmde/getMainSnaks
c6946d0
Added extra filter/map methods to StatementList
@mariushoch
mariushoch merged commit c6946d0 into masterNov 17, 2014
@mariushoch
mariushoch deleted the getMainSnaks branch November 17, 2014 23:25
This was referenced Nov 26, 2014
Merged
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JeroenDeDauw@thiemowmde@mariushoch