[07:35:35] Denny_WMDE: now I want to have getLabel just accept a languagefallbackchain object, saying something like function getLabel( LanguageFallbackChain $chain ) { return $chain->resolveMultilingualData( $this->data['label'] ); } [07:35:48] and the dependency issue seems bigger now [07:36:31] btw. word usage multilanguage vs. multilingual? [07:36:41] aude: ^ [08:54:50] I am going to upgrade Jenkins, would take roughly half an hour to complete. [08:59:46] liangent: you can simply grab all labels [08:59:56] and do the languagefallback outside of entity [08:59:59] keep entity dumb [09:00:19] it would even make sense for languagefallbackchain to simply get all available labels [09:00:25] and then return the right thing [09:00:36] then also the dependency is the right way [09:01:12] DanielK_WMDE_: daily? [09:02:14] liangent: in a minute [09:02:23] multilingual i'd say [09:02:47] daily now for those who want tojoin [09:06:49] liangent: so denny is here for another hour and then out during the afternoon [09:06:57] if you have more questions for him.... [09:12:12] * aude has vim telling me now stuff like "Avoid variables with short names like $rc" :) [09:27:00] * aude waits for vim to tell me how terrible ChangesList in core is! [10:25:07] Does anyone know of an item that uses lots of qualifiers? :) [10:27:48] addshore: http://www.wikidata.org/wiki/Q76 :) [10:27:53] :) [10:41:02] addshore: and note that qualifier modules are out of experimental now [10:41:20] okay :) [10:41:32] statement rank is still experimental [10:44:02] New patchset: Daniel Kinzler; "(bug 50303) improve cache key for shared cache." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71595 [11:02:50] New patchset: Daniel Kinzler; "Re-use top level registries everywhere." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71598 [11:40:15] aude: how do I get started with the mobile entity view? [11:42:57] how about we make a bug ticket for this [11:44:11] okay [11:44:39] then either in the bug comments or in a mediawiki user page, outline more specifically what the class will do? [11:45:08] DanielK_WMDE_ does a good job of detailing how he will implement stuff, in that way [11:45:08] okay [11:45:22] and then give people an opportunity to give feedback [11:45:36] before you spend time, say doing it "wrong" and then have to redo it [11:45:57] this comes under mediawiki extensions right? [11:46:01] and we always break stuff into more specific bug tickets [11:46:17] yes, in bugzilla it's "Wikidata Repo" [11:46:22] (not totally accurate) [11:47:17] okay what if I myself am totally not clear about how I will implement stuff [11:47:23] ok [11:49:19] pragunbhutani: some general principles we aim to follow are http://nikic.github.io/2011/12/27/Dont-be-STUPID-GRASP-SOLID.html [11:50:07] in the case of entity view, it's handling a lot of different stuff [11:50:12] * pragunbhutani reads [11:50:44] if we were to refactor it, then it should be split into multiple classes handling each thing that class does now [11:50:54] one class = one responsibillity [11:50:58] responsibility [11:51:19] hmm [11:51:27] it's the getHtmlForClaims part that is especially an issue in mobile [11:51:57] and it has getHtmlForClaim (for each one) [11:52:40] okay I'm looking at it right now [11:52:41] so maybe we start with something that generates that part of the html. [11:53:13] maybe it should be in a separate class that mobile entity view uses [11:55:31] for the parts that generate the site link section [11:56:01] what is it about getHtmlForClaims that's particularly problematic for mobiles? [11:56:07] it might be more feasible to reuse existing code for generating the site link section html, although i think it should also be split into its own class [11:57:44] pragunbhutani: you see it uses wfTemplate (a template engine in wikibase) and sets css classes, etc. [11:58:04] for mobile these might be different so maybe make it so they are not hard-coded? [11:58:05] yes I see [11:58:14] hmm [11:58:43] make them so that they are assigned automatically according to the device accessing the page? [11:59:01] i would outline some of these specific steps [11:59:27] maybe on a mediawiki user page first and get feedback from wikidata-tech [11:59:40] and getHtmlForClaim could be factored out [12:00:22] i have to think about where to specify the templates and css classes [12:00:54] or wouldn't mind input from others on our team before i tell you to do it the *wrong* way [12:01:28] if we're building another entity view for mobiles specifically, is it not okay to hardcode things? [12:01:46] better not to, generally [12:01:48] OR are we thinking of making just one entity view and controlling how it assigns classes to hings? [12:01:56] *things [12:01:56] i am not sure [12:02:05] yeah me neither [12:02:26] maybe there could be variables in the class that define these things [12:03:14] I think making another entity view just so that different class names can be hardcoded sounds redundant [12:03:24] could be [12:03:43] yes, variables are a way to go [12:03:56] okay so what do I file the bug report as? [12:03:56] * aude needs to poke at the code more and think [12:04:11] "Implement a mobile entity view" ? [12:04:12] how about first step to factor out getHtmlForClaim [12:04:29] okay [12:04:42] and 2 ) document the possible implementation approaches in more detail [12:04:59] 3) maybe also a bug for mobile entity view [12:05:23] I'll file them one by one [12:05:24] 4) link to your document in the bug ticket and email wikidata-tech to solicit feedback [12:05:30] ok [12:05:37] * aude sure this will save time to get feedback in advance [12:05:45] okay :) [12:05:56] vs. have people rage that it was done wrong :o [12:06:48] and maybe just as an experiment, see about getting the claim html to appear in the mobile skin [12:07:06] figure out a way to do that [12:07:42] the getHtmlForClaim? [12:07:46] yes [12:07:51] I'm sure I could give that a shot [12:07:58] that's the smallest piece i think [12:08:25] so ClaimHtmlGenerator or something [12:08:30] * aude not good at naming classes [12:08:39] ClaimHtmlBuilder [12:09:34] but since that function is called by getHtmlForClaims only, would I not need to put that part into the skin as well? [12:10:02] pragunbhutani: perhaps [12:10:10] also, silly to ask but I should know this by now, what are claims? [12:10:39] pragunbhutani: please read up on http://meta.wikimedia.org/wiki/Wikidata/Data_model and look at the WikibaseDataModel extension [12:11:04] okay [12:11:20] the statements on an item page are an extension of the claim object [12:11:29] with references, etc. [12:12:14] ah okay, the property-value pairs [12:12:15] reading [12:12:22] yes [12:12:27] they can have qualifier [12:12:29] s [12:13:15] e.g. used to define the dates someone attended a school (http://www.wikidata.org/wiki/Q76) [12:14:33] okay now, back to the bug report [12:14:40] ok [12:14:41] what can I mention in the description so far [12:14:58] that I'd like to move the getHtmlForClaims part to a mobile skin [12:14:59] ? [12:15:09] no, to it's own class [12:15:19] that mobile skin / view whatever can use [12:15:26] also entity view can use it [12:15:48] oh okay [12:16:13] one class = one responsibility (it does one thing) [12:16:23] or else other developers may rage :) [12:16:32] okay :) [12:21:58] https://bugzilla.wikimedia.org/show_bug.cgi?id=50578 [12:22:20] perfect [12:23:02] i am assigning it to you [12:23:56] create a subpage of my GSoC proposal to document solution approaches [12:24:08] see also https://bugzilla.wikimedia.org/show_bug.cgi?id=40885 [12:24:42] ah yes I was reading that precisely :) [12:24:46] great [12:25:02] i can make your bug a dependency of that one [12:25:12] (one of many dependencies and subtasks that will be required) [12:25:25] also take a look at how stuff is done in js, if you like [12:25:45] yes, it can be made a dependency [12:25:59] all other components would also have to be factored out as dependencies then [12:26:04] yes [12:26:27] at some point we might want even more fine grained rendering of claims [12:26:30] but it makes sense, please do [12:26:37] e.g. depending on what kind of data values they have [12:26:41] hmm [12:26:59] maybe do something different for geocoordinates (e.g. a map) vs. a string or whatnot [12:27:18] getting the claim html thing out is a first step [12:28:15] * aude not expecting you to have different rendering for the data value types but  [12:28:27] but it's a good view to have for the future [12:28:29] to do things in a flexible way that might allow others to do that would be nice to have [12:28:34] yep [12:28:52] where can I look at how stuff is done in the js? [12:28:56] we also have various bug tickets for implementing "formatters" for the data value types [12:29:20] most of the js is currently in lib/resources although there is some "entity view" code in repo/resources [12:29:46] entity view js is glue to put things together, though has its ugliness also [12:30:23] I see some in getHtml [12:38:14] New patchset: Addshore; "Adding and fixing API examples" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71610 [12:39:24] aude: Did you talk to Daniel about the normalization setting yet? [12:39:57] hoo: thanks for reminding me [12:40:04] :) [12:40:11] * aude see if DanielK_WMDE_ is around? [12:41:16] New patchset: Addshore; "Adding and fixing API examples" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71610 [12:47:22] New patchset: Daniel Kinzler; "(bug 49264) Handle bad values using BadValue objects." [mediawiki/extensions/DataValues] (master) - https://gerrit.wikimedia.org/r/70433 [12:53:43] New patchset: Hoo man; "Allow (optional) title normalization in wbgetentities" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [12:54:46] New review: Hoo man; "Don't pass false to SiteLinkCache::getItemIdForLink" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [12:56:00] New patchset: Hoo man; "Use the wbgetentities normalization in jquery.wikibase.linkitem" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71545 [12:59:59] New patchset: Daniel Kinzler; "Merge results in CompositeValidator" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/69845 [13:00:30] New review: Daniel Kinzler; "rebased" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/69845 [13:11:43] aude: what's up? [13:12:04] hi Daniel [13:12:13] DanielK_WMDE_: hoo and i wanted to check about setting the normalise page title setting [13:12:17] normalizeItemByTitlePageNames that is [13:12:21] any reason not to set it to true? [13:14:37] aude: no, i don't think so. [13:14:47] ok [13:14:55] it causes one extra http request when asked for an un-normalized or non-existing title [13:15:03] that should be ok [13:15:08] sure, agree [13:15:17] Yes :) See also https://gerrit.wikimedia.org/r/#/c/71543/2/repo/includes/api/GetEntities.php [13:15:21] and only does that if it does not find a match initially [13:15:31] if there is a match, then no extra http request [13:15:48] exactly [13:16:14] alright, i might not get around to this settings change until tonight [13:16:55] not urgent... I just want it before the above change makes it in [13:17:06] ok [13:19:48] New review: Daniel Kinzler; "(1 comment)" [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/71543 [13:21:09] New patchset: Daniel Kinzler; "(bug 49264) Handle BadValue objects gracefully." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/68002 [13:21:23] New patchset: Siebrand; "Fix gender related i18n issue in anon edit warning" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71613 [13:22:03] New patchset: Siebrand; "Fix gender related i18n issue in anon edit warning" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71613 [13:24:49] New patchset: Daniel Kinzler; "(bug 49264) Make SnakValidator fail on bad values." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/69659 [13:38:39] New review: Jeroen De Dauw; "(1 comment)" [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/71543 [14:01:42] Abraham_WMDE1: hi, congrats. [14:02:11] can you give me link of list of unconnected articles of nl wiki? [14:02:18] New patchset: Daniel Kinzler; "Change client defaults if repo is on same wiki." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70163 [14:03:12] New patchset: Daniel Kinzler; "(bug 49264) Handle bad values using BadValue objects." [mediawiki/extensions/DataValues] (master) - https://gerrit.wikimedia.org/r/70433 [14:05:58] New patchset: Daniel Kinzler; "Change client defaults if repo is on same wiki." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70163 [14:06:12] New patchset: Daniel Kinzler; "Remove hacks from jenkins entry point." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70164 [14:06:25] New patchset: Daniel Kinzler; "(bug 50478) Filter langlinks by site group." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71302 [14:06:49] *phew* [14:06:59] done with follow-ups [14:06:59] now for the reviews... [14:15:04] New review: Daniel Kinzler; "syntax error & indent confusion." [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/71613 [14:36:38] New patchset: Hoo man; "Allow (optional) title normalization in wbgetentities" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [14:38:42] New review: Hoo man; "Addressed Daniel's and Jeroen's comments. Also factored this out into GetEntities::getEntityIds (whi..." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [14:44:03] www.wikidata.org/wiki/Special:Contributions/93.75.134.116 facepalm [14:45:34] fuuuuu gerrit [15:06:04] New patchset: Hoo man; "Allow (optional) title normalization in wbgetentities" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [15:06:34] New review: Jeroen De Dauw; "(1 comment)" [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/71610 [15:11:34] New review: Hoo man; "Fixed" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [15:13:39] New review: Hoo man; "Jeroen?" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [15:17:09] New patchset: Addshore; "Adding and fixing API examples" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71610 [15:20:13] New review: Daniel Kinzler; "I approve of the intention and the code looks sane, but I know too little about the resource loader ..." [mediawiki/extensions/Wikibase] (master) C: 1; - https://gerrit.wikimedia.org/r/70115 [15:25:16] New review: Aude; "(1 comment)" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70163 [15:25:27] JeroenDeDauw: are you still around ? :-) [15:27:45] hashar: yeah [15:28:13] aude: back. was away in past hours [15:28:22] liangent: ok [15:28:31] i am here a little bit more, then home and on again [15:28:52] there were existing functions called *MultilangTexts though [15:29:03] in class Entity [15:29:36] JeroenDeDauw: will you have a bit of time this week to talk about what you could need in wmf Jenkins ? :-] [15:29:53] JeroenDeDauw: I would like to have a bunch of bugs filled so we can start improving Jenkins for your use cases :D [15:30:24] liangent: yes [15:30:36] damn pidgin crashed twice -.- [15:32:20] aude: so have new functions stick to MultilingualTexts and keep existing functions unchanged? [15:32:50] New review: Daniel Kinzler; "Looks sane, but I'm not very happy with the error reporting mode (see my first inline comment). Can ..." [mediawiki/extensions/DataValues] (master) - https://gerrit.wikimedia.org/r/70593 [15:33:24] hoo: use a decent irc client, then [15:33:57] * hoo loves to have IRC and XMPP in one client :P [15:34:38] liangent: you mean for retrieving labels for some set of languages? [15:35:47] or which existing functions? [15:35:54] * aude a little confused [15:36:36] I mean I shouldn't try to rename existing functions, but should use multilingual for new function names. is this correct? [15:36:37] hashar: mailed you [15:38:06] liangent: seems okay but depends case-by-case [15:38:41] in LanguageFallbackChain i am not convinced DanielK_WMDE_ 's suggestions are better but then i'm not good at naming stuff [15:38:54] New review: Daniel Kinzler; "(1 comment)" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/69689 [15:41:24] aude, liangent: most of my comments there are just rants/ideas. take them as inspiration, not requirements. [15:41:30] DanielK_WMDE_: ok [15:41:56] * aude would roll dice and pick a name :) [15:42:13] aude: getSnakeEyes? [15:42:17] :) [15:44:40] JeroenDeDauw: https://gerrit.wikimedia.org/r/71543 ? [15:45:42] hoo: gerrit is being broken for me, cannot see your change [15:46:00] Use anything despite firefox and it works :| [15:46:35] hoo: i think jeroen wants more tests to cover that part you change [15:46:49] * aude not looked at the patch in detail yet [15:47:01] :/ [15:49:53] New review: Daniel Kinzler; "Can be merged except for the attribution issue (see inline comment). " [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/69689 [15:51:07] alright, back in 30 min or so [15:51:11] JeroenDeDauw, hoo: that code relies on external title normalization - we can't do that in a unit test... [15:51:32] (sure, if the nromalization was done via a service object, we could mock it...) [15:52:02] I could of course do it, but I'm anything but sure that's worth the work [15:52:53] hoo: well, it's a complex code path that is currently untested. that's not good. [15:53:39] hoo: actually, normalizePageName has a special "in test" mode where it applies the local wiki's normalization [15:53:43] mh, I could write a basic test which just tests whether the mock normalization for unit tests has been done [15:53:51] you can test based on that... it's not pretty, but it will show whether normalization is applied. [15:53:53] that's what I mean [15:54:08] i think that would be ok [15:55:02] DanielK_WMDE_: What about the normalization setting thing? [15:55:15] Or you could make the code actually testable and maintainable rather then adding more stuff to it and increasing the problem [15:55:54] New review: Daniel Kinzler; "@liangent: please use gerrit's "draft" mode instead of DO NOT MERGE. On the command line, that's git..." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71280 [15:55:58] That would be the nice thing to do; however not a requirement; as long as the new behaviour is tested somehow I'll be happy [15:56:12] Assuming the tests themselves do not form technical debt [15:56:25] hashar: https://gerrit.wikimedia.org/r/#/c/71616/ [15:56:46] JeroenDeDauw: tahksn for the mail :-) will follow up on it either later tonight or tomorrow :) [15:57:32] DanielK_WMDE_: What about the setting? It's not true by default so whatever test I would write couldn't be run on jenkins [15:58:47] hashar: \o/ [15:58:58] and well done on the composer effort :-] [15:59:10] will probably have to talk about the vendor include on the list [15:59:29] I would put it in LocalSettings but there might be a better idea from someone :-] [16:00:40] anyway off, see you tomorrow :-] [16:01:52] New review: Daniel Kinzler; "Looks useful, and the code looks fine. " [mediawiki/extensions/Wikibase] (master) C: 1; - https://gerrit.wikimedia.org/r/71281 [16:04:44] New review: Daniel Kinzler; "agree with denny" [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/71279 [16:04:50] New patchset: Jeroen De Dauw; "Preliminary implementation of DescriptionDeserializer" [mediawiki/extensions/Ask] (master) - https://gerrit.wikimedia.org/r/71631 [16:08:50] DanielK_WMDE_: I don't want to use get*-style function names for resolveMultilingualData [16:09:07] in my view get* is for getting property included in the object itself [16:09:20] New patchset: Henning Snater; "Implemented wikibase label and button widget" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71633 [16:09:47] New review: Daniel Kinzler; "also, needs tests" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71280 [16:14:19] New review: Daniel Kinzler; "Seems fine, lets look again when it has been rebased." [mediawiki/extensions/Wikibase] (master) C: 1; - https://gerrit.wikimedia.org/r/71072 [16:18:23] aude: I think I would only need to move the getHtmlForClaim part into a new class [16:18:42] New review: Daniel Kinzler; "can be simplified" [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/71182 [16:18:51] then the class can be used in entity view and a function can be called in getHtmlForClaims [16:19:12] I'd also like to change the name. getHtmlForClaims and getHtmlForClaim are confusing [16:22:38] pragunbhutani_: refactoring EntityView, and generalizing ValueFormatter so getHtmlForXXX can be moved into individual formatters is already planned (somewhere - i hope we have a bug for that) [16:22:52] it's part of the "write spec for formatters" ticket anyway [16:22:58] please keep an eye opn that [16:24:12] liangent: extract? fetch? whatever. I'm not so much concerned about the verb. the point is that the name should indicate *what* data it returns (namely, the value for the language best matching the fallback chain). [16:25:33] DanielK_WMDE_: we talked a bit about the value formatters [16:25:33] hoo: even if it's not run on jenkins, it'll (hopefully) run when we do repo/client integration test. [16:25:48] hoo: also, jenkins doesn't use plain defaults, it uses the *example* settings. you could turn it on there. [16:25:57] hm, i think it actually *is* turned on there [16:26:08] the idea is to take things piece by piece and also have him draft some plan / spec for how he's approaching stuff [16:26:27] aude: sure, just wanted to make the connection [16:26:33] yes :) [16:26:47] the get html for claim is the most problematic for view in mobile [16:26:53] one thing that has become pretty urgent is formatters that return html though [16:26:54] so it's a good starting point, i think [16:26:58] yes [16:27:18] if no one else picks it up, maybe i can work on it [16:27:24] DanielK_WMDE_: ok :) Will do that later... pushing Fedora 19 to my netbook now [16:27:33] * aude impatient about having stuff appear in diffs, parser function etc [16:28:09] and think liangent also has input for entity view :) [16:29:13] "burn it with fire"? [16:29:25] :) [16:29:43] it's actually not that horrible, just bloated. needs to be split into a dozen pieces [16:30:58] that's the plan [16:33:08] ok, so much for toda's follow-ups and reviews [16:33:17] anyone up to reviewing my stuff? [16:33:21] * DanielK_WMDE_ has ~20 patches gfoing stale [16:34:40] maybe tonight [16:35:03] there is the one about settings that JeroenDeDauw could review maybe? [16:35:05] is it still -2 [16:35:06] ? [16:35:09] New review: Daniel Kinzler; "oops, will fix." [mediawiki/extensions/Wikibase] (master) C: -2; - https://gerrit.wikimedia.org/r/71322 [16:35:28] aude: sure he could [16:35:40] that -2 is quite stale [16:36:05] agree [16:36:12] that would unstick some other patches, i think [16:36:18] there's a bunch that jeroen could review, actually :) [16:36:23] New patchset: Jeroen De Dauw; "Capturing this wisdom in our VCS, for great justice" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71638 [16:36:27] and one about bad values that he had comments on [16:36:47] where i havn't replied? which one? [16:36:57] i thought i went through everything today. or maybe new comments? [16:37:03] DanielK_WMDE_: https://gerrit.wikimedia.org/r/#/c/71638/ :D [16:37:52] JeroenDeDauw: yay for citing out of context! [16:38:13] New review: Jeroen De Dauw; "(1 comment)" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71322 [16:38:41] DanielK_WMDE_: indeed :P [16:39:09] DanielK_WMDE_: though to me it is just as silly in its original context though [16:39:29] New review: Daniel Kinzler; "(1 comment)" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71322 [16:43:33] New patchset: Liangent; "New class LanguageFallbackChain" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70871 [16:45:23] DanielK_WMDE_: MEEEEEEEEEEEERGEEEEEEEEEEEEEEEE https://gerrit.wikimedia.org/r/#/c/70331/ [16:50:34] aude: how does this look? http://www.mediawiki.org/wiki/User:Pragunbhutani/GSoC_2013_Implementation_Approaches#Factor_getHtmlForClaims_out_of_EntityView [16:51:33] * aude clicks [16:53:00] JeroenDeDauw: i guess i'll stare at that on the train tomorrow. or i'll do it in the office where i can annoy you with questions [16:55:03] pragunbhutani_: it's helpful [16:55:15] I know there isn't a lot there [16:55:38] but that's a page I intend to have where I can keep documenting whatever I'm doing [16:56:01] and also sort of have some space for discussion [16:56:15] good [16:56:50] What I'm going to do tonight before going to bed is try and move the getHtmlForClaim to a class of its own [16:57:03] getHtmlForClaims (with an s) stays where it is [16:57:34] I'll see how that turns out and we'll sync at the time of the hangout tomorrow morning [16:57:46] good approach [16:57:51] one thing at a time [16:59:31] all right then, see you tomorrow :) [17:04:15] New patchset: Hoo man; "Allow (optional) title normalization in wbgetentities" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [17:04:30] DanielK_WMDE_: beware, I now got a twice as long blowgun in the office as well, plus extra darts :p [17:05:46] New review: Hoo man; "Added unit tests for normalization" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [17:27:19] New patchset: Liangent; "Enable variant level fallback for {{#property: }}" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71072 [17:46:27] New patchset: Liangent; "Enable variant level fallback for {{#property: }}" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71072 [17:50:14] New patchset: Liangent; "Enable variant level fallback for {{#property: }}" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71072 [17:51:41] New patchset: Liangent; "Enable variant level fallback for {{#property: }}" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71072 [17:54:02] New patchset: Liangent; "Enable variant level fallback for {{#property: }}" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71072 [17:55:34] New patchset: Liangent; "Make MultiLangSerializationOptions aware of fallback chains" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71182 [18:04:47] New patchset: Liangent; "Make MultiLangSerializationOptions aware of fallback chains" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71182 [18:05:16] New review: Liangent; "(1 comment)" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71182 [18:09:40] New patchset: Liangent; "Make MultiLangSerializationOptions aware of fallback chains" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71182 [18:10:02] DanielK_WMDE_: how would you generally consider back compatibility things? [18:10:05] cf. https://gerrit.wikimedia.org/r/#/c/71182/5/lib/includes/serializers/SerializationOptions.php [18:13:21] New patchset: Liangent; "Enable variant level fallback for {{#property: }}" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71072 [18:19:57] New review: Liangent; "(1 comment)" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71280 [18:27:31] liangent: tracking the usage of classes and members is one reason to use an IDE :) that's one click. [18:28:40] DanielK_WMDE_: no. external dependency eg another extension calling it like what we use from Babel [18:29:32] liangent: subclassing it? [18:29:47] would be possible in theory, but unlikely [18:29:53] DanielK_WMDE_: yeah [18:30:21] wikibase is young and a moving target, anything hooking ointo it that deeply would surprise me. [18:30:39] b/c for code interfaces inside wikibase isn't a big concern, less so for protected stuff. [18:30:47] in mediawiki core, it's different, of course [18:30:51] anyway I have been doing work in core :p [18:31:04] also, b/c is important for anything serialized, whether we write it to the database, a file or a cache [18:31:32] liangent: i know you have. i'm just saying: the considerations are different for wikibase [18:31:53] yeah [18:31:56] thousands of extensions exist for mediawiki, hoioking into it in all kinds of manners. [18:32:13] i don't think there is *anything* that depends on Wikibase. [18:35:33] New review: Liangent; "(1 comment)" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71279 [18:48:39] New patchset: Liangent; "Make MultiLangSerializationOptions aware of fallback chains" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71182 [19:02:15] New patchset: Hoo man; "Use the wbgetentities normalization in jquery.wikibase.linkitem" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71545 [19:03:07] New patchset: Liangent; "New function LanguageFallbackChain::newFromContext()" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71279 [19:03:49] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain mostly for confirmation" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [19:10:04] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain mostly for confirmation" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [19:10:33] New patchset: Liangent; "New function LanguageFallbackChain::newFromContext()" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71279 [19:11:00] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain mostly for confirmation" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [19:23:11] is there a namespace / 'package' (java jargon) level variable visibility in php? [19:23:23] variable and function [19:24:33] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain mostly for confirmation" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [19:31:31] liangent: no. just make it public. [19:32:01] liangent: better for testing, too :) [19:32:57] DanielK_WMDE_: can you tweet from your ide? :) [19:34:47] aude: i bet i could :P [19:34:55] heh :) [19:35:01] multichill: hi, [19:35:15] I noticed the bot staled [19:35:27] but I think without interwikis are done [19:35:35] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain mostly for confirmation" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [19:35:54] I have to do other articles [19:36:04] hm. lost some code that i forgot to git add. [19:36:20] and we *really* need to get client tests running on jenkins [19:37:57] i think we are not allowed to merge stuff ourselves now [19:38:07] only jenkins can [19:38:31] * aude wonders how that works in practice 100% of the time [19:39:01] no client tests running = still can merge, but.... [19:39:09] Krinkle set it that way [19:39:13] yep [19:39:19] he thought we are confused by the button [19:39:35] only issue could be backports and such into branches [19:39:50] since tests run with whatever version of that branch + master datavalues [19:40:25] Amir1: No, it stalled at the E [19:40:51] Still plenty of articles which are not in Wikidata [19:41:15] let me check. so there is an error [19:41:15] I managed to get rid of the false positives at nlwp (incomplete tags table) [19:42:38] http://toolserver.org/~multichill/temp/queries/wikidata/articles_not_wikidata.txt is fresh [19:42:47] aude: That only applies to master [19:42:54] hoo: ok [19:43:25] DanielK_WMDE_: I remember you mentioned some "WikibaseRepo-managed factory objects" somewhere [19:43:27] any pointer? [19:44:55] New patchset: Daniel Kinzler; "Use MockSiteStore for testing" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71322 [19:45:37] liangent: look for the class WikibaseRepo resp WikibaseClient. They act as global registries for service objects. The service objects can act as factories/registries. # [19:46:00] A simple example is WikibaseRepo::getDataTypeFactory() [19:46:13] no one for Lib? [19:46:33] multichill: i'm looking for error but output file is so long :/ [19:46:44] Amir1: Tail it? [19:46:50] liangent: there's LibRegistry, but it should be avoided. The idea is: these global registries should only be used where it is otherwise impossible to get the services injected [19:47:12] liangent: by the virtue of being a library, it should be possible to inject the relevant services into everything in lib [19:47:14] yeah but how I can via nano [19:48:13] liangent: so, say, WikibaseRepo has a getLanguageFallbackChainBuilder method, but you need a FallbackChainBuilder in lib. then you class in lib needs to ask for that service in its constructor [19:48:43] so just use new FallbackChainBuilder everywhere in Lib when really needed? [19:49:25] Amir1: Just do tail or tail -1000 |less [19:49:28] and repeat getLanguageFallbackChainBuilder in both WikibaseRepo and WikibaseClient? [19:50:11] oh thank you [19:50:22] I checked and there was no error [19:50:33] I think it couldn't read more [19:51:12] You can just download http://toolserver.org/~multichill/temp/queries/wikidata/articles_not_wikidata.txt and restart it, right? [19:51:27] If it runs on toollabs: That crashed this week [19:51:44] liangent: exactly. the idea is that every class should clearly declare (ideally as constructor arguments) what other (service) objects it needs to collaborate with. [19:51:46] Toollabs: Crashing your programs 10 times faster than the Toolserver! ;-0 [19:52:35] liangent: static scope and "new" calls should only happen at the "edges" of the application - admittedly, mediawiki has a lot of them. [19:52:35] multichill: you're right [19:52:35] I'll restart [19:53:35] New review: Daniel Kinzler; "ok, should actually work now." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71322 [19:57:00] DanielK_WMDE_: what about backcompat things? eg. when I'm given a language code (from existing calls) and I really need a languagefallbackchain [19:57:16] I need to convert the string to languagefallbackchain in the middle [19:58:27] aude: Nice hoax [19:59:44] liangent: we don't have to worry about external things depending on our interfaces. if you change an interface, you can change the callers - if that's not too terribly painful. [19:59:54] heh :D [20:00:27] if the function is used a lot, then i'd suggest to make it accept both (a string or a fallback chain) and fix the callers in a handful of follow-ups. [20:01:11] liangent: you can also use wfWarn to log when the function is called the "old" way, but note that unit tests fail on wfWarn [20:01:21] (use wfDebugLog to avoid that) [20:04:59] DanielK_WMDE_: ok [20:17:38] New patchset: LivingShadow; "RepoApi.js: Add searchEntities()" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71713 [20:17:40] DanielK_WMDE_: and ideally the only static used edges should be WikibaseRepo::getDefaultInstance(), right? [20:18:05] *at edges [20:20:34] why doesn't mediawiki provide a "global" registry for all extensions [20:20:51] bah. btw: had a brief discussion where I ended up defending wikidata vs dbpedia, though tbh I'm not sure of the benefits. [20:22:12] multichill: Creating page Achterlijf (insect) via API [20:22:13] Got an unknown error when putting data: save-failed [20:22:16] :| [20:22:31] some of them has this problem [20:28:54] liangent: WikibaseRepo::getDefaultInstance() is what you would use in a (static) hook handler, or in the constructor of a class that can't take services as pareamters (e.g. API modules, Special pages). [20:29:53] and in other places service instances should be passed around? [20:29:58] DanielK_WMDE_: ^ [20:30:22] or pass the WikibaseRepo instance around? [20:31:33] liangent: no, pass the individual services, not the registry. it's tempting, but bad: depending on the registry means depending on *everything* [20:31:45] and dependencies should be kept minimal. [20:32:27] Amgine: had that several times :) there are two major points, i think: 1) wikidata provides data to wikipedia, dbpedia (painfully) scrapes data from wikipedia. 2) wikidata's data model represents sourced claims, not "facts". [20:32:39] DanielK_WMDE_: dependencies? [20:33:06] liangent: which class knows about which other class [20:33:59] liangent: i fond this article a nice overview of good practices (though i don't agree 100% with everything there): http://googletesting.blogspot.de/2008/08/by-miko-hevery-so-you-decided-to.html [20:34:10] it's not gospel, but has some very good points. [20:52:37] DanielK_WMDE_: what about accepting an optional service instance then? to avoid changing too much code at once. eg function buildSomething( $data, $factory = null ) { [20:52:37] if ( $factory === null ) { $factory = WikibaseRepo::getDefaultInstance()->getMyFactory(); } [20:52:37] return $factory->buildMyObject( $data ); } [20:59:47] liangent: better than nothing, but not really nice. the main problem is that this way, your caller has to know the factory (or you use the default). ideally, only the factory/registry that instanciates your class needs to know what services it needs. [21:00:24] essentially, such parameters in function calls pollute the public interface with internal knowledge about how the function is implemented and what it needs. [21:00:58] thinking in abstract terms, a different implementation might need a completely different set of services to perform the operation [21:01:32] DanielK_WMDE_: yeah if my caller is at edge, it should call WikibaseRepo::getDefaultInstance()->getMyFactory() itself. otherwise it should just pass $factory on [21:01:59] yea, but then you need to pass around the services for all calls - not just for object construction [21:02:04] that's a *lot* more places! [21:02:37] liangent: my approach to incremental refactoring is usually to put the service into a member, and initialize that member in the constructor, using the global registry. [21:03:00] then I put a big TODO there saying that this should really be injected as a parameter to the constructor. [21:03:30] that makes it easy to implement proper injection later, without sprinkeling kludge-code through the classes actual logic and interface [21:03:55] a member of the caller class? [21:04:06] of any class that needs it. [21:04:21] the caller class may not need it, if it's not a function param. [21:11:41] DanielK_WMDE_: https://gerrit.wikimedia.org/r/#/c/71272/ the last comment [21:11:51] about tests again [21:14:54] New patchset: Hoo man; "Allow (optional) title normalization in wbgetentities" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [21:15:28] DanielK_WMDE_: btw the factory object doesn't have private level variable access for data objects, or should I make everything public? [21:15:29] liangent: yea, that's why global state sucks. [21:15:50] it's ok to do it this way. though it sucks that it's needed. [21:15:50] including "internal" members [21:15:58] New review: Hoo man; "Fixed an issue: Only show that we used normalization if the title actually changed. Also added a tes..." [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71543 [21:16:25] liangent: why would the factory need access to private fields in the data object? [21:17:09] DanielK_WMDE_: during construction [21:17:27] or you mean I should get everything ready then feed it to the constructor? [21:17:39] yes, exactly [21:18:04] ok [21:18:28] ideally, a data object is immutable. that's not always practical, but it's a useful goal. [21:24:19] DanielK_WMDE_: leaving now. it could be nice if you can review https://gerrit.wikimedia.org/r/#/c/70871/ + https://gerrit.wikimedia.org/r/#/c/71279/ first and merge them, then move those newFrom* code to a factory class in one single changeset. after moving a bunch of code in https://gerrit.wikimedia.org/r/#/c/70871/ , rebasing https://gerrit.wikimedia.org/r/#/c/71279/ is always a pain [21:25:05] I guess I shouldn't use two changesets at the beginning [21:28:50] DanielK_WMDE_: other patches want review too of course [21:30:58] liangent: you can squash the changesets, if you like [21:31:23] i'll see that i review these first thing tomorrow. which will be around noon, i guess - i'm traveling to berlin tomorrow [21:31:47] * aude would be okay with some stuff being done in follow up [21:32:00] sure, as long as i can see that follow-up [21:32:03] i can't imagine always getting stuff 100% perfect right away [21:32:08] yep [21:32:15] i sure don't :P [21:32:21] it becomes one monster patch [21:36:32] DanielK_WMDE_: I tried to squash it but I can't push it to gerrit now [21:36:39] it says ! [remote rejected] HEAD -> refs/publish/master/FallbackChainFromContext (change 70988 closed) [21:36:52] I don't know how 70988 is connected [21:37:24] yea, easy to get confused there [21:37:54] i usually manage, but i don't think i could give concise instructions [21:41:19] New patchset: Liangent; "New class LanguageFallbackChain" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70871 [21:41:30] reset soft HEAD~1 [21:41:38] git reset soft HEAD~1 and then try again [21:41:44] DanielK_WMDE_: done by cherrypicking it to another local branch [21:41:50] git rebase -i [21:42:07] usually git rebase -i master (what i do) [21:42:09] liangent: one hackish way: check out the second patch, do git reset HEAD^^. that should give you all the changes as local, uncommitted changes [21:42:12] then just make a new commit [21:42:53] aude: yea, interactive reabase is the Right Way, but you have to be careful about the change ID in the final commit message [21:43:01] (and about confusing git-review) [21:43:08] DanielK_WMDE_: yes! [21:43:23] DanielK_WMDE_: I believe I got change id correct [21:43:32] good :) [21:43:48] otherwise it can't explain why it worka after cherry-pick [21:43:51] *works [21:43:53] did you abandon the redundant change? [21:44:10] well, cherry-picking still gives you two commits, right? [21:44:33] did you somehow manage to combine them? [21:44:58] DanielK_WMDE_: I've already squashed with rebase -I [21:45:17] but the result can't be submitted [21:45:29] unless I cherry-pick it to somewhere else [21:45:33] I don't know why [21:46:45] hmm. I imagine git-review is trying to some other commits (incorrectly) at the same time? [21:47:02] and there's no such issue in another branch (i use master) [21:48:50] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain mostly for confirmation" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [21:49:48] really leaving now see you [21:49:56] liangent: yea, that'S what i do: when git-review gets confused (or i do), i cherry-pick into a fresh branch [21:49:58] usually owrks :) [21:50:04] anyway, time for bed [21:50:23] cu [21:57:49] New patchset: Liangent; "New LanguageFallbackChain class" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70871 [21:57:49] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [21:58:20] New patchset: Liangent; "New class LanguageFallbackChain" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/70871 [21:58:34] New patchset: Liangent; "New special page Special:MyLanguageFallbackChain" [mediawiki/extensions/Wikibase] (master) - https://gerrit.wikimedia.org/r/71281 [22:32:34] New review: Hoo man; "Some code style issues, looks good despite (untested)." [mediawiki/extensions/Wikibase] (master) C: -1; - https://gerrit.wikimedia.org/r/71713