Refactored away HashArray #702

JeroenDeDauw · 2017-01-05T21:26:13Z

Removed a bunch of unused code and made SnakList simpler.

SnakList can be improved further in follow ups:

Constructor can hide extending of ArrayObject
Perhaps can use composition of ArrayObject instead of inheritance
removeSnakHash -> removeSnakByHash
...

The changes made so far should not break anything in Wikibase.git, though clearly there are some breaking changes (public methods no longer there and no more HashArray).

thiemowmde

~~I don't feel like this patch can be merged as it is, because it is so big and potentially causes unwanted side effects. I suggest a series of smaller steps before we do this big change:~~

Let's drop as much unused code as we can first. See Drop unused HashArray::removeDuplicates #710, Remove unused HashArray::rebuildIndices and indicesAreUpToDate #711 and Drop "accept duplicates" feature from HashArray #712.
~~Add a constructor in a separate patch, see Add SnakList constructor #439.~~
Make sure SnakListTest covers everything we want it to cover.

thiemowmde · 2017-01-10T16:09:28Z

src/Snak/SnakList.php

+	 *
+	 * @throws InvalidArgumentException
+	 */
+	public function __construct( $input = null, $flags = 0, $iteratorClass = 'ArrayIterator' ) {


In my opinion this constructor is exposing to much (undocumented) complexity that is not needed by users of the SnakList class. Please tell me if I miss something. Otherwise I suggest to merge #439 first and rebase this patch.

I did not touch this in this PR and don't want to add additional changes to it.

I consider the $flags and $iteratorClass parameters implementation details from the ArrayObject class. These parameters where never documented for the SnakList class, and never used, and should not be copy pasted over here in my opinion. As said I would like to merge #439 first.

thiemowmde · 2017-01-10T16:10:18Z

src/Snak/SnakList.php

+	public function __construct( $input = null, $flags = 0, $iteratorClass = 'ArrayIterator' ) {
+		parent::__construct( [], $flags, $iteratorClass );
+
+		if ( $input !== null ) {


This will not be necessary any more with #439.

JeroenDeDauw · 2017-01-11T12:47:09Z

Merge conflict as well

thiemowmde · 2017-01-11T13:14:15Z

I will happily rebase this patch here when the other patches are merged.

JeroenDeDauw · 2017-01-12T20:27:55Z

Merge?

thiemowmde · 2017-01-12T21:04:49Z

#702 (review)?

thiemowmde · 2017-01-13T09:50:34Z

src/Snak/SnakList.php

+			return false;
+		}
+
+		if ( $hasHash ) {


This is now dead code. See #712, where I remove this entire feature from the base class. As before I will happily rebase this patch when #712 is merged.

thiemowmde · 2017-01-13T09:51:17Z

src/Snak/SnakList.php

-	 * @see GenericArrayObject::getObjectType
+	 * Maps snak hashes to their offsets.
+	 *
+	 * @var array [ snak hash (string) => array [ snak offset (string|int) ] | snak offset (string|int) ]


This can not be an array of arrays any more. Multiple is_array checks below are not needed any more. See #712.

thiemowmde · 2017-01-13T14:28:40Z

Rebased. I also inlined what was formerly a protected preSetElement method. This patch is now much closer to being mergable, I believe. Still to do:

Some @since tags are missing.
Make sure the test cases cover everything we want them to cover.
~~Rework the constructor. However, we can do this later, but before doing a release.~~
~~The method name removeSnakHash is new. It's a bit awkward. Suggestion: removeSnakByHash.~~
~~Check if all non-private methods are really needed. Some appear to be duplicates.~~

JeroenDeDauw · 2017-01-13T14:37:25Z

I do not like this "everything needs to be done before this is merged" approach. Not saying it is bad. I find it unpleasant and think its not efficient. I do not want discuss this with you, and I no longer feel like following up on this change either, it's just not fun.

thiemowmde · 2017-01-13T15:09:50Z

Sorry to hear that. All I wanted was a series of smaller patches that are easier to follow, easier to understand and easier to review.

Thanks for merging the patches I did so far.

Removed a bunch of unused code and made SnakList simpler. SnakList can be improved further in follow ups: * Constructor can hide extending of ArrayObject * Perhaps can use composition of ArrayObject instead of inheritance * removeSnakHash -> removeSnakByHash * ...

JeroenDeDauw · 2017-09-13T07:58:27Z

Just noticed this HasArray thing still exists, went looking for a PR that removes it, found this one, and went faceplam. Forgetti much

Since mawster is at 8.0-alpha anyway, I suppose this can be merged right away

Edit: well since it is approved and the reason to not merge it right away is gone, I can just merge it myself

thiemowmde · 2017-09-13T11:42:00Z

Thanks. I updated the release notes in 16f54e9. I used reflection to compile a list of methods SnakList does not support any more.

JeroenDeDauw · 2017-09-13T14:12:54Z

Thanks for updating the release notes

JeroenDeDauw added the breaking label Jan 5, 2017

JeroenDeDauw added this to the 7.0.0 milestone Jan 6, 2017

This was referenced Jan 10, 2017

Drop unused HashArray::removeDuplicates #710

Merged

Remove unused HashArray::rebuildIndices and indicesAreUpToDate #711

Merged

thiemowmde suggested changes Jan 10, 2017

View reviewed changes

thiemowmde force-pushed the rm-hasharray branch 2 times, most recently from fa3d039 to 016649e Compare January 11, 2017 14:15

thiemowmde mentioned this pull request Jan 13, 2017

Drop "accept duplicates" feature from HashArray #712

Merged

thiemowmde suggested changes Jan 13, 2017

View reviewed changes

thiemowmde force-pushed the rm-hasharray branch from 016649e to 666d9f3 Compare January 13, 2017 14:17

This was referenced Jan 14, 2017

Move HashArray::equals to SnakList::equals #713

Merged

Move Hashable interface from HashArray to SnakList #714

Merged

thiemowmde force-pushed the rm-hasharray branch from 666d9f3 to 4cca1e5 Compare February 13, 2017 14:07

thiemowmde force-pushed the rm-hasharray branch 2 times, most recently from 9050061 to ce1b72c Compare March 1, 2017 19:47

thiemowmde force-pushed the rm-hasharray branch from ce1b72c to 75dec09 Compare March 15, 2017 13:25

This was referenced Mar 15, 2017

Do not use SnakList::addElement wmde/WikibaseDataModelSerialization#217

Merged

Do not use dynamic class name in SnakListTest #726

Merged

Inline all abstract stuff from HashArrayTest to the implementations #727

Merged

thiemowmde force-pushed the rm-hasharray branch from 75dec09 to 40b5123 Compare March 16, 2017 13:14

thiemowmde approved these changes Mar 16, 2017

View reviewed changes

thiemowmde force-pushed the rm-hasharray branch from 40b5123 to e23e3c0 Compare September 4, 2017 09:24

JeroenDeDauw modified the milestones: 8.0.0, 7.0.0 Sep 13, 2017

JeroenDeDauw merged commit 0460d67 into master Sep 13, 2017

JeroenDeDauw deleted the rm-hasharray branch September 13, 2017 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored away HashArray #702

Refactored away HashArray #702

JeroenDeDauw commented Jan 5, 2017 •

edited

Loading

thiemowmde left a comment •

edited

Loading

thiemowmde Jan 10, 2017

JeroenDeDauw Jan 11, 2017

thiemowmde Jan 11, 2017

thiemowmde Jan 10, 2017

JeroenDeDauw commented Jan 11, 2017

thiemowmde commented Jan 11, 2017

JeroenDeDauw commented Jan 12, 2017

thiemowmde commented Jan 12, 2017

thiemowmde Jan 13, 2017

thiemowmde Jan 13, 2017

thiemowmde commented Jan 13, 2017 •

edited

Loading

JeroenDeDauw commented Jan 13, 2017

thiemowmde commented Jan 13, 2017

JeroenDeDauw commented Sep 13, 2017 •

edited

Loading

thiemowmde commented Sep 13, 2017

JeroenDeDauw commented Sep 13, 2017

Refactored away HashArray #702

Refactored away HashArray #702

Conversation

JeroenDeDauw commented Jan 5, 2017 • edited Loading

thiemowmde left a comment • edited Loading

Choose a reason for hiding this comment

thiemowmde Jan 10, 2017

Choose a reason for hiding this comment

JeroenDeDauw Jan 11, 2017

Choose a reason for hiding this comment

thiemowmde Jan 11, 2017

Choose a reason for hiding this comment

thiemowmde Jan 10, 2017

Choose a reason for hiding this comment

JeroenDeDauw commented Jan 11, 2017

thiemowmde commented Jan 11, 2017

JeroenDeDauw commented Jan 12, 2017

thiemowmde commented Jan 12, 2017

thiemowmde Jan 13, 2017

Choose a reason for hiding this comment

thiemowmde Jan 13, 2017

Choose a reason for hiding this comment

thiemowmde commented Jan 13, 2017 • edited Loading

JeroenDeDauw commented Jan 13, 2017

thiemowmde commented Jan 13, 2017

JeroenDeDauw commented Sep 13, 2017 • edited Loading

thiemowmde commented Sep 13, 2017

JeroenDeDauw commented Sep 13, 2017

JeroenDeDauw commented Jan 5, 2017 •

edited

Loading

thiemowmde left a comment •

edited

Loading

thiemowmde commented Jan 13, 2017 •

edited

Loading

JeroenDeDauw commented Sep 13, 2017 •

edited

Loading