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.

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

[DNM] Make the whole data model immutable #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

thiemowmde
Copy link
Contributor

No description provided.

@thiemowmde thiemowmde added this to the 3.0.0 milestone Jan 13, 2016
@JeroenDeDauw
Copy link
Contributor

Can we actually get away with this? I'm not up to speed with what we are doing exactly in the frontent, though I'd be surprised if ALL mutability can be removed (in particular that of the higher level aggregates).

@thiemowmde
Copy link
Contributor Author

What do you mean by "higher" lever? The List, Map and Set classes? They are not much more than interfaces and/or abstract base classes for SnakList, StatementList and so on.

But you are right, at this point I do not know if this kills our UI code. This still needs investigation where all these methods are used.

@thiemowmde thiemowmde changed the title Make the whole data model immutable [DNM] Make the whole data model immutable Jan 14, 2016
@thiemowmde thiemowmde changed the title [DNM] Make the whole data model immutable Make the whole data model immutable Jan 25, 2016
@thiemowmde
Copy link
Contributor Author

I did a fulltext search for the removed method names in all JavaScript code I have in my local environment. Result: Most are unused, except for what I'm listing below.

  • Fingerprint.setAliases is used in AliasesChanger. This bit of code is really weird and most probably a mistake, because no other changer does that.
  • SnakList.move, SnakList.moveDown and SnakList.moveUp are used in jquery.wikibase.snaklistview.
  • SnakList.merge is used multiple times, at least in jquery.wikibase.referenceview and jquery.wikibase.statementview.
  • I can confirm that all setItem are unused.
  • There are 5 removeItem methods removed in this patch. It seems some of them are used, but this needs more investigation because there is an unrelated removeItem in jquery.wikibase.listview.
  • Similar with addItem. It's used at least once, but this is hard to see because there are other methods with the same name.

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Wikibase that referenced this pull request Jan 28, 2016
This is in preparation for
wmde/WikibaseDataModelJavaScript#54

Change-Id: I835f6a383305170d2173cdd194c104377765af21
@thiemowmde thiemowmde removed this from the 3.0.0 milestone Jan 28, 2016
@Benestar
Copy link
Contributor

Is this still desired and are you still working on this? You removed this patch from the 3.0 milestone but not #40 so I wonder if I should continue working on that or if we are removing most of the methods anyway.

@thiemowmde
Copy link
Contributor Author

This is a proof-of-concept. It will probably not happen in the next big release, because the add and remove methods seem to be used. It's also not so critical, since all critical methods are unused anyway.

@thiemowmde
Copy link
Contributor Author

I reverted (almost) all changes to the base interfaces and classes Group, GroupableCollection, List, Map and Set. This should make this much easier to work on. We can remove this in a later patch, if we want.

@thiemowmde thiemowmde changed the title Make the whole data model immutable [DNM] Make the whole data model immutable Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants