[07:12:48] New patchset: Netbrain; "added a more robust value checking on MapsLocation data" [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12170 [07:23:08] New patchset: Netbrain; "Added a special value "on" for visitedicon." [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12176 [07:30:47] New patchset: Netbrain; "Added a special value "on" for visitedicon." [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12176 [07:31:24] New review: Netbrain; "Had to rebase due to dependency i didn't notice." [mediawiki/extensions/Maps] (master) C: 0; - https://gerrit.wikimedia.org/r/12176 [09:02:43] I hate non-threadsafe SQL [12:46:20] New review: Jeroen De Dauw; "(no comment)" [mediawiki/extensions/Maps] (master); V: 1 C: 2; - https://gerrit.wikimedia.org/r/12176 [12:46:22] Change merged: Jeroen De Dauw; [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12176 [13:28:16] New review: Netbrain; "This is ready for review." [mediawiki/extensions/Maps] (master) C: 1; - https://gerrit.wikimedia.org/r/12170 [13:47:01] New review: Jeroen De Dauw; "(no comment)" [mediawiki/extensions/Maps] (master); V: 1 C: 2; - https://gerrit.wikimedia.org/r/12170 [13:47:05] Change merged: Jeroen De Dauw; [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12170 [15:41:47] New patchset: Yaron Koren; "Changed '!==' to '!=' for 'distributionlimit' check - use of '!==' always seems to cause problems" [mediawiki/extensions/SemanticMediaWiki] (master) - https://gerrit.wikimedia.org/r/12386 [15:52:34] New review: Jeroen De Dauw; "Why did the old code not work? What value did it get? An empty string? Null? In either case we shoul..." [mediawiki/extensions/SemanticMediaWiki] (master); V: 0 C: -2; - https://gerrit.wikimedia.org/r/12386 [16:41:49] New patchset: Nischayn22; "Added DIHandlers Classes and required methods" [mediawiki/extensions/SemanticMediaWiki] (storerewrite) - https://gerrit.wikimedia.org/r/12397 [17:25:16] JeroenDeDauw: I am having a hard time figuring out this signature stuff in the compatibility helpers, are you aware of that code [17:46:22] nischayn22: I am yes [17:46:43] nischayn22: what do you want me to do first? Look at your earlier commit or try to help you w/ this? [17:47:34] JeroenDeDauw: you can help better after looking at that commit :) [17:47:58] nischayn22: ok looking now [17:53:10] New review: Jeroen De Dauw; "Looks good, just one minor issue with the exception type." [mediawiki/extensions/SemanticMediaWiki] (storerewrite); V: 0 C: -1; - https://gerrit.wikimedia.org/r/12397 [17:53:32] nischayn22: ^ [17:53:50] JeroenDeDauw: looking [17:55:01] JeroenDeDauw: I have reused that exception code from Compatibility Helpers [17:55:18] nischayn22: any specific questions about the signature stuff? What are you having problems with? [17:55:36] nischayn22: right ok, but the comment is still valid :) [17:56:13] nischayn22: after merging in this change I'll pull the branch locally and have a better look at the stuff [17:56:13] JeroenDeDauw: I did a grep on getSignatureFromDataItemId and saw that it is used at only two places, but couldn't get its use [17:56:55] JeroenDeDauw: this change makes some unstable changes :( [17:57:52] nischayn22: are you making any changes to it? [17:57:56] \If not I'll merge it now [17:58:10] JeroenDeDauw: I am adding the since tags [17:58:15] Ok [17:58:26] I only see getSignatureFromDataItemId used in once place [17:58:45] SQLStore2::getTypeSignature [17:59:08] Should be pretty trivial to update the code there and use your new stuff [17:59:13] Or wait [17:59:21] We want to get rid of it all right? :p [17:59:54] Hmm [18:00:05] Looks like that value is actually not used at all anywhere [18:00:08] wtf [18:01:55] . [18:02:13] Without breaking anything (or at least that's what I think I wanted to say) [18:03:33] New patchset: Nischayn22; "Added DIHandlers Classes and required methods" [mediawiki/extensions/SemanticMediaWiki] (storerewrite) - https://gerrit.wikimedia.org/r/12397 [18:04:21] JeroenDeDauw: Its use is suspicious :P [18:04:42] Huh>? [18:04:52] Do you see it used anywhere? [18:05:51] I am actually tempted to just kill it on master and see if stuff still works [18:05:56] ofc w/o unit tests that's sort of flaky [18:06:13] JeroenDeDauw1: no, that's why I wonder why is it there and Markus explained me so deeply like it was something very important [18:06:31] I only understood little about it :p [18:07:48] JeroenDeDauw1: also have a look at getIndexFromDataItemId() , I don't get that one too [18:08:04] it returns hardcoded stuff which makes it difficult to understand [18:12:50] nischayn22: eating now - back in 20 min or so] [18:13:03] JeroenDeDauw1: Ok [18:24:34] New patchset: Foxtrott; "fix bug: menuselect - list of values visible" [mediawiki/extensions/SemanticFormsInputs] (master) - https://gerrit.wikimedia.org/r/12438 [18:28:02] New review: Markus Kroetzsch; "Do we actually need a handler for DIError? It cannot be stored and queried." [mediawiki/extensions/SemanticMediaWiki] (storerewrite); V: 0 C: -1; - https://gerrit.wikimedia.org/r/12397 [18:33:39] nischayn22: I don't know about getIndexFromDataItemId actually [18:33:42] Lookign at it now [18:34:31] JeroenDeDauw1: Markus finally did a review today [18:34:46] I saw [18:35:26] getIndexFromDataItemId not making much sense to me after looking [18:35:39] JeroenDeDauw1: same here :p [18:35:51] nischayn22: I asked him to join IRC but then he went offline :p [18:35:53] it has something to do with sorting/selecting [18:36:05] Yeah [18:36:22] Why don't you address his comments - then I'll merge the stuff [18:36:26] And have a closer look [18:36:34] JeroenDeDauw1: Markus explained to me about it, but its not easy from the code [18:36:41] JeroenDeDauw1: sure, one moment [18:53:27] New patchset: Demon; "Adding .gitreview" [mediawiki/extensions/SemanticACL] (master) - https://gerrit.wikimedia.org/r/12457 [19:02:00] New patchset: Nischayn22; "Added DIHandlers Classes and required methods" [mediawiki/extensions/SemanticMediaWiki] (storerewrite) - https://gerrit.wikimedia.org/r/12397 [19:03:32] JeroenDeDauw1: done with the copy+paste stuff [19:08:33] New review: Jeroen De Dauw; "(no comment)" [mediawiki/extensions/SemanticMediaWiki] (storerewrite); V: 1 C: 2; - https://gerrit.wikimedia.org/r/12397 [19:08:36] Change merged: Jeroen De Dauw; [mediawiki/extensions/SemanticMediaWiki] (storerewrite) - https://gerrit.wikimedia.org/r/12397 [19:10:34] New review: Jeroen De Dauw; "(no comment)" [mediawiki/extensions/SemanticFormsInputs] (master) C: 1; - https://gerrit.wikimedia.org/r/12438 [19:13:43] JeroenDeDauw1: Thanks for reviewing. Waiting for confirmation from the reporter, will then merge. [19:14:01] Ftrott: np [19:23:12] New patchset: Jeroen De Dauw; "Some cleanup" [mediawiki/extensions/SemanticMediaWiki] (storerewrite) - https://gerrit.wikimedia.org/r/12463 [19:24:06] nischayn22: https://gerrit.wikimedia.org/r/#/c/12463/ [19:24:11] nischayn22: some cleanup [19:24:22] Please close stuff such as arrays on the same line you begin them [19:24:25] Much clearer [19:24:40] Also, if you have arrays with lots of stuff, put one item per line [19:24:49] Putting multiple really makes it unreadable [19:25:36] JeroenDeDauw1: I do such mistakes when copying code, will be careful next time [19:26:18] New patchset: Jeroen De Dauw; "Some cleanup" [mediawiki/extensions/SemanticMediaWiki] (storerewrite) - https://gerrit.wikimedia.org/r/12463 [19:26:43] nischayn22: also try to use type hinting wherever possible [19:26:57] Although it'd be better to have interfaces to hint against then classes [19:27:03] But we can easily change this later on [19:28:13] JeroenDeDauw1: yes, I was not sure of these classes' design [19:28:39] I was thinking of having SMWDIHandler as an abstract class at first [19:29:23] nischayn22: nah, current code looks good to me in that regard [19:29:56] We have a bunch of stuff in SMWDataValueFactory that should probably be moved to the DIHandler class [19:30:03] Meh [19:30:05] All static [19:30:22] Seriously, that should all have been non-static [19:30:27] And use a singleton [19:30:37] Now it caches stuff in static fields [19:30:46] Which is as evil as it gets [19:31:01] * JeroenDeDauw1 makes a mental note to rage a bit to markus [19:31:52] nischayn22: the code looks reasonable enough to me [19:32:00] JeroenDeDauw1: I am really new to design patterns [19:32:33] I heard about singleton few weeks back for the first time :P [19:32:44] nischayn22: I'd be helpfull if you gave me a quick overview of what your changes are supposed to do already, where you are going, and what you need my help with [19:32:58] JeroenDeDauw1: Ok [19:33:02] nischayn22: don't worry, if you ever want to feel very smart about the code you wrote, look at Maps 0.1 :) [19:33:18] heh [19:33:36] JeroenDeDauw1: let me explain what I want to achieve (in storerewrite) [19:34:12] we need a way to store information about fields of a DI type in Handlers [19:34:35] Does SMW still work with the storerewrite branch? And to what extend is the enw stuff already used? [19:34:38] so we can modify them as per need, For example Boolean uses two fields like Numbers which is not necessary [19:34:58] nischayn22: heh - that's odd :) [19:35:08] nischayn22: but yeah, makes sense to clean that up [19:35:16] Does not really affect the overall design though [19:35:20] JeroenDeDauw1: It works except a small bug (only one I have noticed so far) [19:35:27] Only whatever migration script you come up with [19:35:49] Having tests would actually be great [19:36:05] JeroenDeDauw1: an udpate would be enough I think, why migrate? [19:36:07] Even if your code as a whole does not work yet, you can very certain components do [19:36:54] JeroenDeDauw1: so, similarly with String types, they can be stored better [19:37:18] nischayn22: you're right - we could just have people drop their old tables and do a full rebuild of their semantic data - that makes a lot more sense then what I was thinking of :) [19:37:52] so we need to remove the use of Compatibility Helpers and get information about table structure from DIHandlers [19:37:53] nischayn22: changes to individual types like that are pretty isolated and do not affect the rest of the work to be done [19:38:09] nischayn22: I can give you feedback on them or discuss them if you want though [19:38:22] nischayn22: yeah, this stuff should go :) [19:38:38] Actually documented to be removed in 1.7 if I'm not mistaken, which clearly did not happen [19:38:38] JeroenDeDauw1: I too thought of migration script, it will be efficient, but too much effor :p [19:38:59] Yeah, no need for a migration script [19:39:42] nischayn22: so to what extend is your code used already? [19:39:53] Are the new tables already used? [19:40:04] I'm wondering how I can best test your code [19:40:22] JeroenDeDauw1: there aren't any new tables yet [19:40:30] I have only renamed them [19:40:57] their redesign can only be done after the code doesn't depend much on Helpers [19:41:14] JeroenDeDauw1: I removed use of getDBkeysFromDataItem in my last commit , as you might have seen [19:41:24] similarly I have to remove in other places [19:41:50] but I am not sure what getIndexFromDataItemId is meant to do yet [19:42:07] even after Markus explained me once [19:42:12] nischayn22: re your earlier remark of making SMWDataItemHandler abstract. I just noticed that the DIHandlers are actually deriving from it. In that case it does make sense to make it abstract and put in abstract methods for getTableFields, getWhereConds and getInsertValues [19:42:22] Please do that [19:42:56] JeroenDeDauw1: It is deriving now, that can also be removed I think [19:43:25] depends on what further methods will need, I am not sure about that yet [19:43:34] nischayn22: Having a base class might very well turn out to be useful in the future [19:43:40] So the deriving part is good [19:43:54] nischayn22: however, I just reconsidered about the abstract methods [19:43:56] I thought so too, it generalizes stuff [19:44:01] It would be better to create an interface [19:44:06] That holds these 3 methods [19:44:13] And is then implemented by your abstract base class [19:44:20] JeroenDeDauw1: Right, I will do that [19:44:37] I actually had that earlier :p [19:44:55] nischayn22: so yeah, the base class is not strictly needed now, but I suspect it will turn out to be in the future, so having it now will save refactoring [19:45:02] nischayn22: what, the interface? [19:45:26] yes, I had made abstract base methods at first [19:45:37] Right [19:45:59] Let's have a further look at getIndexFromDataItemId later [19:46:07] What's the bug you found? [19:46:38] The DIHandlers return fields for that DI type [19:47:07] but some special properties have different fields, as they have different tables (like spec2) [19:47:44] the Helpers aren't aware of the fields in these tables [19:47:54] so we need to figure something out [19:48:17] I am thinking of using the same type of tables for special properties as well [19:48:21] will ask Markus [19:49:16] nischayn22: right [19:49:37] New review: Jeroen De Dauw; "(no comment)" [mediawiki/extensions/SemanticMediaWiki] (storerewrite); V: 1 C: 2; - https://gerrit.wikimedia.org/r/12463 [19:49:38] I think we can handle that somehow [19:49:39] Change merged: Jeroen De Dauw; [mediawiki/extensions/SemanticMediaWiki] (storerewrite) - https://gerrit.wikimedia.org/r/12463 [19:49:52] nischayn22: testing with this branch now [19:50:10] JeroenDeDauw1: testing by installing? [19:50:13] nischayn22: btw, if you are blocked somehow, you can always write tests for the stuff you created [19:50:29] nischayn22: testing by using the code on my existing SMW install [19:50:42] nischayn22: tests will really help you out :) [19:50:52] JeroenDeDauw1: You will need to update the tables [19:51:00] JeroenDeDauw1: I see how tests are useful now [19:51:46] they let you check all features at one go, rather than what I have been doing with my wiki :p [19:51:54] nischayn22: http://pastebin.com/zW7HBjvY [19:52:21] Oh, so you did change the tables already? [19:53:29] JeroenDeDauw1: I changed their names :p [19:54:46] JeroenDeDauw1: talking to markus about the bug [19:55:20] **** [19:55:36] Looks like the update script no longer property hooks into MWs update script :/ [19:58:18] nischayn22: ok, ran the update script [19:58:23] it ran w/o issues [19:58:31] But now when I try to refresh my data I get this http://dpaste.org/NIw6m/ [19:58:57] JeroenDeDauw1: that's the bug I am talking about [20:00:23] JeroenDeDauw1: if you undo the my last commit, then the update would run [20:09:10] nischayn22: right [20:09:21] nischayn22: try to keep your branch close to master [20:09:34] nischayn22: if you never update with the changes from master, merging in things will become hard [20:09:41] nischayn22: you can do this using git rebase [20:09:43] or git merge [20:09:49] Although you probably want to use rebase [20:09:54] JeroenDeDauw1: I did a squash few days back right [20:10:07] Oh [20:10:13] I just did a rebase [20:10:19] And got a pile of conflicts in the messages file [20:11:00] JeroenDeDauw1: I don't see much acitivity on master except message files [20:11:26] nischayn22: indeed [20:11:54] JeroenDeDauw1: so if messages conflict we should always use the ones from master right? [20:12:42] nischayn22: that depends [20:12:50] nischayn22: if you did not make any changes to the messages, then yes [20:13:02] then git is just failing and conflicting for no good reason :p [20:13:04] Which happen... [20:13:08] *happens [20:13:16] nischayn22: did you make any message changes? [20:13:26] JeroenDeDauw1: I don't remember any such [20:22:38] nischayn22: then it is indeed very trivial to fix the conflicts [20:22:52] In fact, you could just use the file from master :) [20:23:08] JeroenDeDauw1: ofc :) [20:23:38] JeroenDeDauw1: what are some important tests we need? [20:23:59] do we need one for setuptables? [20:36:49] New patchset: Jeroen De Dauw; "Fixed automatic running of SMW updating script when running maitenance/update.php" [mediawiki/extensions/SemanticMediaWiki] (master) - https://gerrit.wikimedia.org/r/12516 [20:37:35] nischayn22: care to do a review? :) [20:40:42] JeroenDeDauw1: ok [20:45:35] nischayn22: just noticed that SMWDIHandlerWikiPage has a store field [20:46:00] JeroenDeDauw1: yes it needed that to make the SMW id [20:46:07] nischayn22: probably make sense to put this in the base class [20:46:24] now we have a use for it already :p [20:47:42] nischayn22: re tests: I'd just go create a test for every new method you created. So have a test class for each class in which you added or really changed a method, and for each method have a test method [20:47:46] New review: Reedy; "(no comment)" [mediawiki/extensions/SemanticACL] (master); V: 1 C: 2; - https://gerrit.wikimedia.org/r/12457 [20:47:48] Change merged: Reedy; [mediawiki/extensions/SemanticACL] (master) - https://gerrit.wikimedia.org/r/12457 [20:47:50] Which just tests something [20:47:57] In some cases there is very little to test [20:48:15] But you can even simply test if the return value is of the correct type [20:48:25] Or if you feed it something, that it does not crash [20:49:02] nischayn22: every method excusing those that are duplicated somewhat, such as the getTableFields one [20:49:07] Would be silly to go test them all [20:49:24] Better to just construct some DIHandler via the factory method and test that method in it [20:51:12] JeroenDeDauw: re the review, I don't get much of that stuff [20:51:52] nischayn22: of what stuff? My explanation? Or how to write the tests? [20:52:19] JeroenDeDauw: reviewing your recent commit [20:52:30] JeroenDeDauw: https://gerrit.wikimedia.org/r/#/c/12516/ [20:55:13] New patchset: Reedy; "Add .gitignore" [mediawiki/extensions/SemanticACL] (master) - https://gerrit.wikimedia.org/r/12525 [20:56:19] New review: Reedy; "(no comment)" [mediawiki/extensions/SemanticACL] (master); V: 1 C: 2; - https://gerrit.wikimedia.org/r/12525 [20:56:20] Change merged: Reedy; [mediawiki/extensions/SemanticACL] (master) - https://gerrit.wikimedia.org/r/12525 [21:20:20] nischayn22: feel free to either +1 or -1 my commit :) [21:20:29] nischayn22: I'm off for today, ttyl [22:10:13] New patchset: Reedy; "Bug 37783 - Add DE Layer NS" [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12544 [22:14:55] New review: Siebrand; "Please add the actually requested translation." [mediawiki/extensions/Maps] (master); V: 0 C: -1; - https://gerrit.wikimedia.org/r/12544 [22:17:05] New patchset: Reedy; "Bug 37783 - Add DE Layer NS" [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12544 [23:36:31] New review: Jeroen De Dauw; "(no comment)" [mediawiki/extensions/Maps] (master); V: 1 C: 2; - https://gerrit.wikimedia.org/r/12544 [23:36:33] Change merged: Jeroen De Dauw; [mediawiki/extensions/Maps] (master) - https://gerrit.wikimedia.org/r/12544