Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

API byIDs added to ArrayList #3828

Merged
merged 1 commit into from
Apr 26, 2016
Merged

API byIDs added to ArrayList #3828

merged 1 commit into from
Apr 26, 2016

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented Jan 26, 2015

Added byIDs for consistent API with DataList

@dhensby dhensby mentioned this pull request Jan 26, 2015
Merged
@dhensby
Copy link
Contributor Author

dhensby commented Jan 26, 2015

@silverstripe/core-team to be honest, I'm thinking that this function byIDs on DataList/ArrayList should just be deprecated.

It's a function that is effectively a hard coded filter which doesn't add any real benefit over doing $this->filter('ID', $ids);

Having it as part of the code base increases code maintenance and doesn't really provide any special flexibility.

@tractorcow
Copy link
Contributor

It's probably ok, since filtering by ID (in DataList) negates the necessity to do a classwalk to find the table for the filtered field. It doesn't really give the same benefit in ArrayList as it does there though.

halkyon
halkyon reviewed Jan 26, 2015
View reviewed changes
tests/model/ArrayListTest.php
@@ -746,7 +746,8 @@ public function testCanFilterByEmpty() {
$this->assertFalse($list->canFilterBy('Age'));
}

public function testByID() {
public function testByID()
{
Copy link
Contributor

Choose a reason for hiding this comment

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

super nitpicky mode
brace shouldn't be on a newline ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

I had setenv nitpickymode 0, but yeah I saw that. :)

Welcome back @halkyon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPStorm! Rage! Can I get away with saying.... PSR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't it you who added the .editorconfig files @dhensby ? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately you can't dictate code style like this, it's more white space based. :(

@tractorcow
Copy link
Contributor

The issue with travis is caused by travis-support generating the incorrect version tags for framework on the 3 branch (it should be 3.x-dev not dev-3). I'll look into this later. :D

@dhensby dhensby force-pushed the pulls/byids-arraylist branch 2 times, most recently from 0a91fbe to a6aeb87 Compare January 27, 2015 10:19
@dhensby
Copy link
Contributor Author

dhensby commented Jan 27, 2015

Ok, great. I've fixed that naughty curly brace going to the wrong line and I've also squashed the tests

@dhensby
Copy link
Contributor Author

dhensby commented Feb 3, 2015

Ok, so I noticed that the SS_Filterable interface has diverged a bit from what's implemented on DataList and ArrayList so I've updated the interface and implemented the method(s) as needed on ArrayList.

I've also opened issue #3847 so we can have some clarity on what should be returned by the map method.

As I've now edited the interface, I think that either this needs to go against master OR I need to pull out the interface changes and add them to master.

@dhensby
Copy link
Contributor Author

dhensby commented Feb 3, 2015

As a compromise we could put all these changes into 3 (except the interface changes) and then put the interface changes into 4.

Therefore we get a consistent API which is BC for 3.2 and then enforce the interface properly (non-BC) in 4?

@dhensby
Copy link
Contributor Author

dhensby commented Feb 17, 2015

bump @silverstripe/core-team

@dhensby dhensby force-pushed the pulls/byids-arraylist branch 2 times, most recently from aeb49b9 to ec897ce Compare August 28, 2015 14:50
@dhensby dhensby mentioned this pull request Aug 28, 2015
Closed
@dhensby dhensby force-pushed the pulls/byids-arraylist branch 3 times, most recently from f8e5625 to d148804 Compare August 28, 2015 15:57
@camspiers
Copy link
Contributor

As mentioned on #4557 I think there is a mistake in ArrayList::filterAny, and I think tests should be added for it.

tractorcow
tractorcow reviewed Aug 31, 2015
View reviewed changes
model/ArrayList.php

}

protected function normaliseFilterArgs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

phpdoc please.

@dhensby
Copy link
Contributor Author

dhensby commented Sep 2, 2015

feedback dealt with

camspiers
camspiers reviewed Sep 2, 2015
View reviewed changes
model/ArrayList.php
$itemsToKeep = array();

foreach ($this->items as $item) {
$keepItem = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you can remove this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, thanks :)

@tractorcow
Copy link
Contributor

Dealings-with with fed-back

@dhensby dhensby mentioned this pull request Jan 25, 2016
Open
@sminnee
Copy link
Member

sminnee commented Apr 19, 2016

Hey @dhensby @tractorcow where are we at with this?

@tractorcow tractorcow merged commit da8f4a7 into silverstripe:3 Apr 26, 2016
@tractorcow
Copy link
Contributor

We are at merged.

@dhensby dhensby deleted the pulls/byids-arraylist branch April 26, 2016 09:50
@dhensby
Copy link
Contributor Author

dhensby commented Apr 26, 2016

yay, thanks

@camspiers
Copy link
Contributor

Nice work @dhensby!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dhensby @tractorcow @camspiers @sminnee @halkyon @stevie-mayhew