[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
daniel added a comment. @Thiemo: I'm missing numbers for the Term, TermList, and TermFallback classes. Did these slip through the cracks somehow? TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: thiemowmde, daniel Cc: Tobi_WMDE_SW, daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs, Mbch331 ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
thiemowmde added a comment. Not sure how this happened. AliasGroupList, Fingerprint, and all Term classes where missing. I updated my list above. TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: thiemowmde Cc: Tobi_WMDE_SW, daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs, Mbch331 ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
thiemowmde added a comment. The task description talked about "data model objects", so I simply did the same steps for all classes in the current https://phabricator.wikimedia.org/tag/wikibase-datamodel/ 4.4.0 (dev). TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: thiemowmde Cc: Tobi_WMDE_SW, daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs, Mbch331 ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
JeroenDeDauw added a comment. Thanks for the overview Thiemo! Note that you included some services, while this task is about value objects, aggregates and entities. TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: thiemowmde, JeroenDeDauw Cc: Tobi_WMDE_SW, daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs, Mbch331 ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
thiemowmde added a comment. I did regex searches for `new Class(` instantiations as well as static `Class::…(` calls in my local copy of the codebase, including all dependencies (ValueView, DataModel, all DataValues, and so on). I did not included other extensions like PropertySuggester or WikibaseQuality. I listed all usages no matter in which (sub) component they are. I tried to **highlight** what I think is a possible issue and should, for example, use a factory instead of creating an object directly. - `AliasGroup` - Instantiations in production: AliasGroupList, AliasGroupListDeserializer, Fingerprint, LegacyFingerprintDeserializer - Usage in tests: 122 - `BasicEntityIdParser` - Instantiations in production: dumpEntities, EntityChange, SqlEntityInfoBuilder - Static calls in production: **WikibaseClient, WikibaseRepo** - Usage in tests: 75 - `ByPropertyIdArray` - Instantiations in production: ChangeOpStatement - Usage in tests: 0 - `Claims` - Usage: 0 - `DerivedPropertyValueSnak` - Usage: 0 - `DispatchingEntityIdParser` - Instantiations in production: BasicEntityIdParser, **WikibaseClient, WikibaseRepo** - Usage in tests: 0 - `EntityIdParsingException` - Instantiations in production: DispatchingEntityIdParser, ItemIdParser, **populateChangesSubscription**, SuffixEntityIdParser - Usage in tests: EntityIdDeserializerTest - `EntityIdValue` - Instantiations in production: EntityIdValueParser - Usage in tests: 44 - `EntityRedirect` - Instantiations in production: EntityContent, EntityContentDataCodec, EntityDataRequestHandler, RedirectCreationInteractor - Usage in tests: 52 - `HashableObjectStorage` - Usage: 0 - `ItemId` - Instantiations in production: BasicEntityIdParser, CachingSiteLinkLookup, createBlacklistedItems, DiffView, ExternalChangeFactory, HashSiteLinkStore, ItemHandler, ItemIdParser, LegacySiteLinkListDeserializer, MergeItems, ModifyEntity, SiteLinkListPatcher, SpecialPagesWithBadges, SpecialSetSiteLink, UpdateRepoJob, WikibaseLuaBindings - Static calls in production: ChangesSubscriptionTableBuilder, EntityPerPageTable, Item, LegacyIdInterpreter, SiteLinkTable, SiteLinkUniquenessValidator, SiteLinkUsageLookup - Usage in tests: 1034 - `ItemIdParser` - Usage: 0 - `ItemIdSet` - Instantiations in production: SiteLink - Usage in tests: SiteLinkListTest, SiteLinkTest - `Item` - Instantiations in production: createBlacklistedItems, GenericEntityInfoBuilder, Item, ItemContent, ItemDeserializer, ItemDiffer, ItemHandler, LegacyItemDeserializer, LinkTitles, SpecialNewItem - Usage in tests: 429 - `LegacyIdInterpreter` - Static calls in production: EntityContentDataCodec, EntityIdValue, EntityPerPageTable, LegacyEntityIdDeserializer, SqlEntityInfoBuilder, TermIndexEntry - Usage in tests: TermIndexEntryTest - `MapValueHasher` - Instantiations in production: HashableObjectStorage, HashArray, SnakList - Usage in tests: 0 - `PropertyId` - Instantiations in production: BasicEntityIdParser, ByPropertyIdArray, ByPropertyIdGrouper, CallbackFactory, changePropertyDataType, PropertyHandler, PropertyIdResolver, SnakObject, WikibaseRepo - Static calls in production: LegacyIdInterpreter, Property, PropertyInfoTableBuilder, SnakObject, SpecialListProperties, SqlEntityInfoBuilder - Usage in tests: 545 - `Property` - Instantiations in production: LegacyPropertyDeserializer - Static calls in production: importProperties, PropertyContent, PropertyDeserializer, PropertyHandler, SpecialNewProperty - Usage in tests: 114 - `PropertyNoValueSnak` - Instantiations in production: LegacySnakDeserializer, SnakDeserializer, SnakFactory - Usage in tests: 432 - `PropertySomeValueSnak` - Instantiations in production: LegacySnakDeserializer, SnakDeserializer, SnakFactory - Usage in tests: 110 - `PropertyValueSnak` - Instantiations in production: DerivedPropertyValueSnak, LegacySnakDeserializer, SnakDeserializer, SnakFactory - Usage in tests: 264 - `ReferencedStatementFilter` - Usage in production: 0 - Usage in tests: StatementListTest - `Reference` - Instantiations in production: LegacyStatementDeserializer, ReferenceDeserializer, ReferenceList, SetReference - Usage in tests: 143 - `ReferenceList` - Instantiations in production: **ClaimDiffer**, LegacyStatementDeserializer, ReferenceListDeserializer, Statement, StatementList - Usage in tests: 70 - `SiteLink` - Instantiations in production: DeletePageNoticeCreator, InfoActionHookHandler, LegacySiteLinkListDeserializer, LinkTitles, MovePageNotice, OtherProjectsSidebarGenerator, Runner, SiteLinkDeserializer, SiteLinkList, SiteLinkTable, SiteLinkUniquenessValidator, UpdateRepo, UpdateRepoOnMoveJob, WikibaseLuaBindings - Usage in tests: 155 - `SiteLinkList` - Instantiations in production: Item, LegacySiteLinkListDeserializer, LinkTitles, SetSiteLink, SiteLinkListPatcher - Usage in tests: 54 - `SnakList` - Instantiations in production: ClaimDiffer,
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
Tobi_WMDE_SW added a subscriber: Tobi_WMDE_SW. Tobi_WMDE_SW added a comment. This should be extended to everything that is included in the Wikidata build (e.g. serializers, etc..). We also need this for our tests (ideally separated). TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: Bene, Tobi_WMDE_SW Cc: Tobi_WMDE_SW, daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
JeroenDeDauw added a comment. > Those aren't important for the current RFC I was under the impression the proposal was to have interfaces for all value objects (and presumably collections) in DataModel. If that is the case, we should consider the cost of changes to all of them. If not, then I'm rather concerned about making such disruptive changes for just part of the DataModel, forcing us to do more breaking of stuff as soon as we want another alternate implementation. > Yes, but fixing the tests should be very easy Do you see a difference between the tests and the production code? TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: Bene, JeroenDeDauw Cc: daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
JeroenDeDauw added a comment. Thanks for looking at this. Those findings are not surprising - such construction ought to occur primarily in the data access layer and deserialization code. Any reason you did not include `EntityId` and derivatives? Why did you exclude the tests? If I'm not mistaken the reason for having this investigation task is to get an idea of the cost of making breaking changes. That cost applies just as well to test code as to production code. What do you mean with "our codebase"? Which code did you include in this analysis? Is it everything that needs to be maintained by the Wikidata team, or just Client, Lib and Repository? TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: Bene, JeroenDeDauw Cc: daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
Bene added a comment. In https://phabricator.wikimedia.org/T112893#1657772, @JeroenDeDauw wrote: > Thanks for looking at this. Those findings are not surprising - such > construction ought to occur primarily in the data access layer and > deserialization code. Indeed, our code seems to handle the construction of value objects in a very nice way. > Any reason you did not include `EntityId` and derivatives? Those aren't important for the current RFC that is adressed by this investigation because we already have an abstract base interface with `EntityId`. > Why did you exclude the tests? If I'm not mistaken the reason for having this > investigation task is to get an idea of the cost of making breaking changes. > That cost applies just as well to test code as to production code. Yes, but fixing the tests should be very easy in most cases by replacing the constructor calls to some random implementation. However, it would be nice if all the tests would cover all implementations of the data model that we are going to implement. Just for record: There are of course a lot of constructor calls in the tests. > What do you mean with "our codebase"? Which code did you include in this > analysis? Is it everything that needs to be maintained by the Wikidata team, > or just Client, Lib and Repository? For now, I only looked into the Wikibase.git repository and its dependencies but we also need to check our other extensions. We should add the usages from there to the list. TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: Bene Cc: daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
daniel added a comment. I think it's useful to have separate numbers for the tests, but I agree that we should know those numbers too, since we need to fix these instances too. TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: Bene, daniel Cc: daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
[Wikidata-bugs] [Maniphest] [Commented On] T112893: [Task] Investigate how and where data model objects are instanciated in our code base
Bene added a comment. Some statistics (test files excluded): - `SnakObject`/`PropertyValueSnak` constructor calls (4 usages): - repo: SnakFactory - others: SnakDeserializer, DerivedPropertyValueSnak, LegacySnakDeserializer - `SiteLink` constructor calls (19 usages): - client: ClientHooks, PropertyParserFunction\Runner, WikibaseLuaBindings, DeletePageNoticeCreator, InfoActionHookHandler, MovePageNotice, OtherProjectsSidebarGenerator, UpdateRepo - lib: SiteLinkTable - repo: LinkTitles, UpdateRepoOnMoveJob, SiteLinkUniquenessValidator - others: SiteLinkDeserializer, SiteLinkList, LegacySiteLinkListDeserializer - `Term` constructor calls (11 usages): - lib: TermIndexEntry, LanguageFallbackLabelDescriptionLookup - repo: EntitySearchHelper - others: TermDeserializer, LanguageLabelDescriptionLookup, Fingerprint, TermList, LegacyFingerprintDeserializer - `AliasGroup` constructor calls (4 usages): - others: AliasGroupListDeserializer, AliasGroupList, Fingerprint, LegacyFingerprintDeserializer.php TASK DETAIL https://phabricator.wikimedia.org/T112893 EMAIL PREFERENCES https://phabricator.wikimedia.org/settings/panel/emailpreferences/ To: Bene Cc: daniel, Lydia_Pintscher, Aklapper, JanZerebecki, aude, Bene, JeroenDeDauw, thiemowmde, Jonas, Wikidata-bugs ___ Wikidata-bugs mailing list Wikidata-bugs@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
