[04:18:32] I will let you know when I see infobliss around here [04:18:32] @notify infobliss [09:35:26] hello [09:35:31] hi [09:35:58] did you get a chance to look at the code? [09:36:07] yeah, but too unreadable [09:36:13] can you flake8 [09:36:39] is flake8 done manually? [09:36:46] ? [09:37:13] flake8 will report a ton of issues with your code. you gotta fix them one by one [09:37:15] I mean is there an automated way to to conform to flake8 rules? [09:37:22] idk [09:37:33] there may be but I never used any [09:37:43] ok [09:37:51] * zhuyifei1999_ always does it manually :P [09:38:06] ok [09:40:07] just fyi, if you were someone else that I don't mentor or have friendship or other relationship with, I would simply refuse to read the code if it's not flake8-ed [09:41:21] (or other linting for other languages) [09:42:02] once a friend of mine asked me to look at his html. the first thing I did: correct indents [09:42:53] oh the third ticket I talked about last time is now public: https://phabricator.wikimedia.org/T169957 [09:43:06] ok [10:26:47] infobliss: could you ping me when you get it flake8-ed? [10:26:56] I'll reread the code [13:42:51] I will let you know when I see infobliss around here [13:42:51] @notify infobliss [15:22:54] hi it may already be sleep time for you [15:23:05] there were indeed too many readability issues [15:23:16] I got them fixed [15:23:32] I'm still here [15:23:34] barring a few in NationaalArchiefGLAM.py [15:23:48] oh you are here :) [15:24:12] https://github.com/infobliss/sibutest2/tree/master/OOP [15:24:19] please check [15:25:07] a sec [15:27:30] Please leave your review here. It's dinner time for me. I will be back in 15 minutes and check. [15:29:11] https://github.com/infobliss/sibutest2/blob/master/OOP/GenericGLAM.py#L9 if you are writing docs here, shorten it to one line. make detailed docs in the "body", with one empty line separation between title and body, like a git commit messgae [15:29:24] https://github.com/infobliss/sibutest2/blob/master/OOP/GenericGLAM.py#L12 the same applies [15:29:47] https://github.com/infobliss/sibutest2/blob/master/OOP/GenericGLAM.py#L15 if you are adding a comment, use # [15:29:59] this function already has docs [15:30:30] https://github.com/infobliss/sibutest2/blob/master/OOP/GenericGLAM.py#L16 wikitext will be logged instead of displayed to user. you should return it [15:30:53] https://github.com/infobliss/sibutest2/blob/master/OOP/GenericGLAM.py#L11 else? [15:31:04] brb [15:33:56] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L12 <= literally json.loads(urllib2.urlopen(url).read()) [15:34:20] I don't see the point of return 1, "" [15:36:06] I'm pretty sure I complained about this function a while ago, let me search the logs [15:37:40] https://wm-bot.wmflabs.org/logs/%23wikimedia-sibu/20170626.txt [15:38:03] basically all the code around it does no good but harm, imo [15:38:53] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L24 add a linebreak after { and dedent the following lines [15:39:10] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L80 why is this a string? [15:39:28] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L85 if photographerName in photographersAnefo: [15:40:19] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L86 what is this? explain why this needs to return three values [15:41:01] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L96 why is this a class variable? is it used? [15:41:35] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L98 literally return load_json_from_url(url) [15:42:11] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L113 if this is a comment use # [15:42:36] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L110 try: except: [15:43:05] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L117 if creator in [.., .., ..]: [15:43:57] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L137 does this even work? [15:44:08] dict is a built in class [15:44:17] you need an instance of it [15:44:42] and you dhouldn't have name collisions with builtins [15:45:12] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L139 if this is a comment use # [15:45:55] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L140 dict['title'] = parsed_j["doc"]["Titel"] or 'zonder titel' [15:46:15] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L144 if this is a comment use # [15:46:35] also conventionally captialize TODO: [15:47:14] alright [15:47:34] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L146 same [15:47:48] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L151 ... [15:48:11] why are there so many constants? [15:48:35] initialize the dict with them, or use dict.update [15:49:05] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L164 collapse these lines [15:49:14] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L169 same [15:49:37] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L177 if parsed_j["doc"]["auteursrechten_voorwaarde_Public_Domain"]: [15:50:36] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L199 what's the point? categories is [category] [15:51:01] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L210 same comment issue [15:51:16] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L211 return? [15:51:59] https://github.com/infobliss/sibutest2/blob/master/OOP/infobox_templates.py#L111 I still think these two functions are too simple to be functions [15:52:25] https://github.com/infobliss/sibutest2/blob/master/OOP/infobox_templates.py#L1 add a linebreak after { and dedent the following lines [15:52:35] https://github.com/infobliss/sibutest2/blob/master/OOP/infobox_templates.py#L53 same [15:53:22] ok I think I finished first round of yelling at the code :P [15:54:20] thanks :) [15:55:33] how do I avoid writing this in a single line? [15:55:36] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L80 [15:56:01] I might need two rounds of style fixes before I start yelling at the actual oop logic :P [15:56:24] is photographersNotAnefo a dict or string? [15:56:33] dict [15:57:19] photographersNotAnefo = { [15:57:19] 'Harry Pot': 'Harry Pot‎', [15:57:19] 'Poll, Willem van de': 'Willem van de Poll' [15:57:19] } [15:57:30] oh ok [15:57:47] the .keys() is useless [15:59:06] I don't get you [15:59:14] can we get the code styling finished today? I'm a little worried about the progress over the last couple of days [15:59:27] if photographerName in photographersAnefo.keys(): [15:59:31] is the same as [15:59:34] if photographerName in photographersAnefo: [15:59:55] ok [16:00:28] and honestly, I'm not a huge fan of if-statements [16:00:46] can we get the code styling finished today? <= you mean fixing these issues you have pointed out [16:01:20] I should have more once you done these [16:01:47] yeah I will do it today [16:02:06] cuz it's hard to focus on the less notable stuffs when there are more issues in plain sight [16:02:40] yeah sure [16:03:00] @seen basvb [16:03:13] wm-bot3: hello? [16:03:40] "Last time I saw basvb they were quitting the network with reason: Quit: Page closed N/A at 6/28/2017 6:09:29 PM (9d21h53m55s ago)" [16:03:54] I will let you know when I see basvb around here [16:03:54] @notify basvb [16:04:12] I'm not a huge fan of if-statements <= what alternative do you suggest? [16:04:58] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L24 make this dict values tuples [16:05:31] then return photographers.get(photographerName) [16:06:33] honestly, I would rewrite the whole thing if I were you [16:07:38] alright. no problem. [16:07:43] I will rewrite [16:08:02] fix the problems I mentioned earlier first [16:08:19] ok [16:08:52] I will point out where things can be refactored later [16:09:15] ok [16:16:34] oh sorry that I have a habit of nitpicking about code. I don't feel good when some python code does not follow 'the zen of python' [16:16:43] https://www.irccloud.com/pastebin/lYYmQ9w7/ [16:19:36] https://en.wikipedia.org/wiki/Zen_of_Python [16:21:00] btw: you ever say this? https://www.python.org/doc/humor/#python-block-delimited-notation-parsing-explained [16:21:46] *saw [16:27:28] nope [16:27:39] very funny [16:28:34] haha yes [16:31:05] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L16 [16:31:28] yeah? [16:31:42] which exception should be raised if the url is invalid? [16:32:41] nothing, .read() and json.loads will raise the exception themselves if necessary [16:34:47] you said "<= literally json.loads(urllib2.urlopen(url).read())" [16:34:55] so in the try block I have this [16:35:14] yeah the whole function is json.loads(urllib2.urlopen(url).read()) [16:35:20] the rest is pointless [16:35:25] imo [16:35:53] ok what do I have in the except block? [16:36:23] nothing, I don't see the point of the try block [16:36:43] if I were you I don't think I would even have that function [16:37:07] got you [16:45:40] How long are you here today? [16:45:55] idk [16:45:59] I might sleep soon [16:46:08] alright [16:46:19] I will finish it in half an hour or so [16:46:22] k [16:46:27] you may check tomorrow morning [16:46:34] just wondering, what text editor do you use? [16:46:41] gedit [16:46:50] well, I kind of want to see it finished today [16:46:54] uh [16:47:14] gedit doesn't feature a lot of stuffs [16:47:38] o.0 [16:48:08] eg select an area, press tab, indent all [16:49:23] hmm ok [16:49:51] and I have flake8 intergrated into my text editor :) [16:50:32] which one is it? [16:50:37] sublimetext? [16:51:17] atom, but it's slower and takes more time to configure than sublime text [16:51:27] but I like atom because it's open source [16:51:44] ok. One thing. If I make it a tuple I will lose the ability to get the correct photographer name right? [16:51:45] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L24 make this dict values tuples [16:52:08] ? [16:52:24] what do you mean by "ability to get the correct photographer name"? [16:52:46] I mean [16:53:05] from the parsed json I get the key value [16:53:17] from which I get the photographer name from the dict [16:53:56] actually, I don't get why this should ever be hard-coded [16:54:38] by converting to tuples I meant 'Zeylemaker, Co / Anefo': (True, 'Co Zeylemaker') [16:54:48] something like that [16:55:03] and that true /false represent Anefo or not [16:55:12] yes [16:55:22] but I don't understand why these should ever be hard coded [16:55:32] maybe ask basvb next time [16:57:08] so you are suggesting to keep these photographer names in a separate file? [16:57:36] yeah [16:57:52] but let's ask basvb about it next time [16:58:15] oh, always separate code and data [16:58:28] he has said to me that this part was ad-hoc for getting work done [16:59:07] I'll ask them myself [16:59:26] * zhuyifei1999_ hates hard-coding [17:34:31] infobliss: how's it going? [17:36:19] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L211 Is it necessary to return? [17:36:31] I am done [17:37:13] well, how do you want to use the filled template? [17:38:11] yes [17:38:22] in the base class only [17:38:40] ? [17:39:08] oh ok [17:39:15] in the base class you printed it https://github.com/infobliss/sibutest2/blob/master/OOP/GenericGLAM.py#L16 [17:39:17] so return the wikitext [17:39:29] which will basically log it and does nothing [17:39:36] yeah [17:39:42] no I will return it in the base class now [17:40:35] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L199 what's the point? categories is [category] <= actually it is necessary to amke it a list [17:40:45] yeah if it's returned in base class you will have to return in all calling methods as well [17:40:55] sure [17:40:59] huh? [17:41:32] btw, category_text is unused [17:41:40] wait a sec [17:42:08] yeah it will be sent to the dictionary [17:43:37] yeah so [17:43:58] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L199 is to check if any category is given by enduser [17:44:57] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L164 collapse these lines <= meaning? [17:45:35] imo, generate the wikitext and let end user add categories manually [17:45:53] a sec [17:47:39] Oh that is an alternative. [17:47:50] something like: [17:47:59] https://www.irccloud.com/pastebin/IM099rFq/ [17:51:27] ok [17:51:33] give me 2 mins [17:51:38] I will post new code [17:51:43] k [17:57:06] oh [17:57:11] ? [17:58:12] just a sec [17:59:14] https://codeshare.io/Gbrwzb [17:59:29] Is this fine for putting the constant values into the dict? [17:59:53] just use simple { [17:59:58] not dict( [18:05:04] https://github.com/infobliss/sibutest2/tree/master/OOP [18:05:10] rewritten [18:05:22] looking [18:06:04] https://github.com/infobliss/sibutest2/blob/master/OOP/GenericGLAM.py#L11 flake8 would complain about missing space after # [18:06:54] https://github.com/infobliss/sibutest2/blob/master/OOP/infobox_templates.py#L1 still, enter, dedent [18:07:33] flake8 would complain about never import * [18:07:40] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L3 [18:08:31] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L93 what's 1 for? [18:09:04] return 1 if unable to decide which is the right template [18:09:35] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L99 june 26 complaint https://wm-bot.wmflabs.org/logs/%23wikimedia-sibu/20170626.txt [18:09:36] in such a case Photograph template will be default [18:10:03] I mean yeah I have seen this. [18:10:15] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L121 if creater in [.., .., ..]: [18:10:31] yeah? [18:10:48] well, never do bare except: [18:10:54] except Exception: [18:11:12] that's what I was asking [18:11:20] ? [18:11:21] which exception should be raised? [18:11:35] like IOError etc [18:11:55] they will all be subclasses of Exception [18:12:02] so except Exception: [18:12:14] oh I see. [18:12:19] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L138 noop? [18:12:40] but not all raisables are exceotions [18:13:06] eg os.exit will raise SystemExit, subclass of BaseException [18:13:21] that will not get caught by except Exception [18:13:55] but they are rare used in extreme cases to indicate "don't catch me" [18:14:22] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L145 TODO [18:14:40] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L147 collapse this [18:16:04] and flake8 the code, sorry [18:17:05] sorry [18:17:09] I was in a hurry [18:17:27] also could you get use either single quotes or double quotes consistently? [18:17:42] alright [18:17:44] I personally prefer single quotes [18:18:56] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L93 <= unable to decide? raise an exception [18:19:10] don't use flags like 0 or 1 in return values [18:21:26] I mean it's not an exception per se. It's a flag yeah. [18:21:53] why not? [18:22:21] so it means that use photograph template for this image [18:22:43] then return 'Photograph' [18:23:01] or use super [18:24:00] ok [18:26:09] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L138 [18:26:22] Is this wrong? [18:26:46] isn't it photographer = creator? [18:27:00] what's str() for here? [18:27:35] may be there were some issue [18:27:41] It's Basvb's code [18:27:47] I will check myself [18:27:58] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L140 <= +=? [18:28:41] anyways, this part of code is horrifying [18:28:42] oh yes [18:29:01] tons of flags [18:30:49] https://github.com/infobliss/sibutest2/blob/master/OOP/NationaalArchiefGLAM.py#L143 comment [18:31:05] * zhuyifei1999_ needs to sleep [18:31:27] ok sure [18:31:36] btw which part is horrifying? [18:31:36] I'll leave more notes here if you post your changes [18:31:47] I will rewrite [18:32:19] line 119-141 + line 12-81 [18:32:33] I'll rewrite it myself [18:32:55] umm ok [18:33:09] It's Basvb's code though :) [18:33:14] yeah ik [18:33:33] definitely not the prettiest code in the world :P [18:34:05] that sounds a lot better to me :) [18:34:22] I didn't read their code in detail due to it not flake8-ed [18:34:37] but actually reading it, argh [18:35:17] I can see that [18:35:31] please post your changes asap, I'll base by rewrite off that [18:35:45] yeah m going to do it now [18:35:49] k [18:35:52] * zhuyifei1999_ zzz [18:36:04] ok you may sleep and check in the morning [20:34:13] Rewritten code [20:34:15] https://github.com/infobliss/sibutest2/tree/master/OOP