1 2012-01-07T01:39:28 *** qxcv
2 2012-01-07T01:56:40 *** raignarok
3 2012-01-07T03:01:39 *** qxcv
4 2012-01-07T03:42:42 <bilal> Hi all, would it be okay if I do issue #143 for a "Find a bug, fix a bug (easy)" task?
5 2012-01-07T03:42:47 <bilal> GCI task: http://www.google-melange.com/gci/task/view/google/gci2011/7236216
6 2012-01-07T03:42:54 <bilal> Issue: https://bitbucket.org/thomaswaldmann/moin-2.0/issue/143/html-validation-errors-are-generated-by
7 2012-01-07T03:45:49 *** MattMaker
8 2012-01-07T07:43:30 *** qxcv
9 2012-01-07T10:04:33 *** greg_f
10 2012-01-07T11:04:33 *** raignarok
11 2012-01-07T11:30:22 *** Dragooon
12 2012-01-07T11:31:26 <Dragooon> Hello
13 2012-01-07T11:31:29 <Dragooon> Anyone around?
14 2012-01-07T11:51:57 *** qxcv
15 2012-01-07T12:05:56 *** qxcv
16 2012-01-07T12:31:38 <TheSheep> Dragooon: no, just us, mice
17 2012-01-07T12:31:48 <Dragooon> Haha
18 2012-01-07T12:31:56 <Dragooon> Are you a mentor/developer?
19 2012-01-07T12:32:06 <TheSheep> yes
20 2012-01-07T12:32:31 <Dragooon> Oh okay, I'm fixing a bug related to mediawiki link parsing
21 2012-01-07T12:32:46 <Dragooon> let me find the link
22 2012-01-07T12:32:53 <Dragooon> https://bitbucket.org/thomaswaldmann/moin-2.0/issue/134/mediawiki-parser-throws-exception-on-file
23 2012-01-07T12:33:20 <Dragooon> Now, the fix itself is easy but since it was more of a workaround to an already fairly ugly piece of code, ThomasWaldmann said it would be better to refactor it
24 2012-01-07T12:33:26 <Dragooon> That part is easy, I get it.
25 2012-01-07T12:33:48 <Dragooon> Now, from what I'm seeing, file link parser doesn't support all mediawiki link args?
26 2012-01-07T12:34:04 <Dragooon> like I can't specify [[File:file.png|link=test]] and get it linked to test
27 2012-01-07T12:36:06 <TheSheep> You want to add that?
28 2012-01-07T12:36:16 <Dragooon> Can I?
29 2012-01-07T12:37:05 <TheSheep> That would be a separate ticket though. I think it would be best to refactor the code to fix the original issue, leaving room for the additional features, and then, in a separate commit, add the additiotonal features
30 2012-01-07T12:37:31 <Dragooon> I see, okay
31 2012-01-07T12:37:36 <TheSheep> Dragooon: you are a google student, right?
32 2012-01-07T12:37:41 <Dragooon> Yes
33 2012-01-07T12:38:10 <TheSheep> I'm not sure, but I think we can make a task for improving the link parser
34 2012-01-07T12:38:27 <TheSheep> then you will get credit for it too :)
35 2012-01-07T12:39:49 <Dragooon> I can create a ticket and make it a bug?
36 2012-01-07T12:39:55 <Dragooon> Because afaik you can't add tasks now
37 2012-01-07T12:41:00 <Dragooon> Regardless, this is fun. My main intention of working with Moin is to gain some Python experience
38 2012-01-07T12:41:28 <TheSheep> that's awesome
39 2012-01-07T12:41:34 <TheSheep> sure, create a ticket
40 2012-01-07T12:42:16 <Dragooon> Thanks, i'll do that after this task
41 2012-01-07T12:42:26 <Dragooon> Plus being safe at first gives me a lot of breathing room
42 2012-01-07T13:01:11 <Dragooon> There are multiple bugs in the mediawiki parser
43 2012-01-07T13:01:32 <Dragooon> Hmm…maybe not
44 2012-01-07T13:14:02 <izibi> bilal: i'm already working on this bug
45 2012-01-07T13:14:51 *** raignarok
46 2012-01-07T13:16:23 <Dragooon> izibi: Was that for me?
47 2012-01-07T13:17:47 <izibi> Dragooon: no, it's about bug #143
48 2012-01-07T13:18:11 <Dragooon> ok
49 2012-01-07T13:22:34 <Dragooon> TheSheep: I refactored the logic and all the tests pass, that means that it's fine right?
50 2012-01-07T13:24:14 <TheSheep> Dragooon: yes
51 2012-01-07T13:24:25 <TheSheep> Dragooon: assuming that part had good test coverage
52 2012-01-07T13:24:26 <Dragooon> Can you review?
53 2012-01-07T13:24:30 <Dragooon> Seems to
54 2012-01-07T13:24:36 <TheSheep> Dragooon: link?
55 2012-01-07T13:24:41 <Dragooon> Just a sec, uploading
56 2012-01-07T13:26:20 <Dragooon> TheSheep: http://codereview.appspot.com/5517058/
57 2012-01-07T13:26:37 <Dragooon> Wait, ignore _args.py
58 2012-01-07T13:27:47 <Dragooon> TheSheep: Okay, noow it should be fine
59 2012-01-07T13:27:49 <Dragooon> now*
60 2012-01-07T13:33:02 <TheSheep> Dragooon: can you add a test with more than one link_args? Because I think there is something fishy in that branch
61 2012-01-07T13:33:49 <Dragooon> TheSheep: Yea it's screwed atm
62 2012-01-07T13:33:50 <TheSheep> Dragooon: also, I would use link_args_list for the already split list of args
63 2012-01-07T13:34:12 <TheSheep> Dragooon: so tha tyou don't use the same variable for a string and for a list
64 2012-01-07T13:34:51 <Dragooon> so..link_args = parse_arguement(….link_args_list[:-1])?
65 2012-01-07T13:35:30 <TheSheep> link_args_list = link_args[1:].split('|')
66 2012-01-07T13:35:58 <Dragooon> Yeah I get that, but below where it's parsed into arguments
67 2012-01-07T13:37:16 <TheSheep> yeah, I would use a different name there too, 'args' is free :)
68 2012-01-07T13:37:38 <Dragooon> parsed_args?
69 2012-01-07T13:37:52 <TheSheep> even better
70 2012-01-07T13:39:49 <Dragooon> Oh crap I just reverted the changes
71 2012-01-07T13:40:10 <TheSheep> Dragooon: it would also be more readable if you added a comment with an example of how the string looks like
72 2012-01-07T13:40:33 <TheSheep> Dragooon: because right now I have to guess why you are removing the first character, for example
73 2012-01-07T13:40:36 <Dragooon> TheSheep: The mediawiki parser has multiple bugs, including caption parsing, link handling
74 2012-01-07T13:40:51 <Dragooon> So I'll claim different tasks for them and fix accordingly
75 2012-01-07T13:40:53 <Dragooon> Is that fine?
76 2012-01-07T13:40:56 <Dragooon> TheSheep: Yeah, okay
77 2012-01-07T13:42:35 * Dragooon may not be point hungry but he wil not miss an oppurtunity
78 2012-01-07T13:43:13 *** raignarok
79 2012-01-07T13:43:42 <Dragooon> TheSheep: uploaded a new patch
80 2012-01-07T13:53:13 <Dragooon> TheSheep: What timezone are you in?
81 2012-01-07T13:55:26 <TheSheep> Dragooon: CET
82 2012-01-07T13:55:43 <TheSheep> Dragooon: sorry, got distracted
83 2012-01-07T13:55:56 <Dragooon> Ah, okay
84 2012-01-07T13:56:09 <TheSheep> Dragooon: sure, it's ok, it's easier to work on one piece of code
85 2012-01-07T13:56:55 <Dragooon> Okay
86 2012-01-07T13:59:14 <TheSheep> Dragooon: you removed the tests?
87 2012-01-07T13:59:24 <Dragooon> No?
88 2012-01-07T13:59:33 <Dragooon> Oh damn
89 2012-01-07T13:59:54 <Dragooon> I reverted, forgot to undo
90 2012-01-07T14:00:05 * Dragooon is dumb
91 2012-01-07T14:00:21 <Dragooon> uploaded
92 2012-01-07T14:00:22 <TheSheep> happens all the time to the best
93 2012-01-07T14:02:03 <TheSheep> thanks
94 2012-01-07T14:02:10 <Dragooon> Is the code fine?
95 2012-01-07T14:11:18 <Dragooon> TheSheep: ^
96 2012-01-07T14:52:37 *** qxcv
97 2012-01-07T15:00:03 *** yufra
98 2012-01-07T15:01:59 *** yufra
99 2012-01-07T15:04:11 <Dragooon> TheSheep: there?
100 2012-01-07T15:06:48 <Dragooon> dreimark, eSyr, ThomasWaldmann : Anyone of you here?
101 2012-01-07T16:43:22 *** RogerHaase
102 2012-01-07T17:02:58 <Dragooon> Does Moin? have a in-built validator for http links?
103 2012-01-07T17:41:59 *** raignarok
104 2012-01-07T18:04:16 <Dragooon> Hello
105 2012-01-07T18:04:24 <Dragooon> Anybody around?
106 2012-01-07T18:47:22 *** greg_f
107 2012-01-07T19:41:36 *** Dragooon
108 2012-01-07T21:46:02 *** MattMaker
109 2012-01-07T22:15:39 *** raignarok
110 2012-01-07T22:36:17 <ThomasWaldmann> re
111 2012-01-07T22:45:38 <izibi> wb
112 2012-01-07T22:50:57 <izibi> ThomasWaldmann: can you review this please? http://codereview.appspot.com/5522054/#ps3001
113 2012-01-07T23:00:38 <ThomasWaldmann> did you discuss it with jek?
114 2012-01-07T23:07:27 <izibi> well he added a bug report to flatland
115 2012-01-07T23:08:14 <izibi> and we both agree that adding the value to the id makes sense at least in the case of radio buttons and sometimes (but now for the usersettings) also for checkboxes
116 2012-01-07T23:08:20 <izibi> s/now/not/
117 2012-01-07T23:10:52 <ThomasWaldmann> but what you are working around in your changeset is not quite what is described in the flatland issue
118 2012-01-07T23:12:55 <ThomasWaldmann> your changeset is only about checkboxes, the issue is only about radiobuttons
119 2012-01-07T23:13:23 <izibi> #9 <label> for= generation needs to know about radio/*checkbox* id mangling
120 2012-01-07T23:13:53 <izibi> the exact same thing also applies for checkboxen, he just used a radio button as an example there
121 2012-01-07T23:14:12 <izibi> *checkboxes
122 2012-01-07T23:14:59 <ThomasWaldmann> and he suggests that the issue is in the for= generation, not in the input field id
123 2012-01-07T23:15:59 <ThomasWaldmann> and if one uses a radio button, it will still break although you said you are working around that issue
124 2012-01-07T23:16:21 <izibi> i guess he still has to figure out how to solve this as the label generator doesn't even know about the field type ;)
125 2012-01-07T23:18:34 <izibi> for radio buttons you need different ids because you (usually) always have multiple radio buttons with the same name as this is what radio buttons are designed for ;)
126 2012-01-07T23:20:54 <izibi> you probably won't be able to use the label generator for radio buttons anyways
127 2012-01-07T23:21:32 <izibi> because you either want a label for every radio button or one for all (for which you might want to use fieldsets i guess)
128 2012-01-07T23:23:08 <ThomasWaldmann> well, i just think either the issue or your source code comments need to be clarified
129 2012-01-07T23:31:40 <izibi> ThomasWaldmann: Flatland adds the value to the ID of checkboxes which we do not want as the for attributes of the labels will point to a non-existent ID in that case. To fix this we manually generate the ID for checkboxes here. related issue in flatland: https://bitbucket.org/jek/flatland/issue/9
130 2012-01-07T23:31:46 <izibi> would this comment be better?
131 2012-01-07T23:34:58 <ThomasWaldmann> yup
132 2012-01-07T23:36:21 <izibi> anything else? what about the placeholder thing? is it ok to just change the type to text?
133 2012-01-07T23:36:51 <ThomasWaldmann> do whatever works best
134 2012-01-07T23:39:51 <izibi> updated the issue
135 2012-01-07T23:40:29 <izibi> everything ok nowß
136 2012-01-07T23:40:31 <izibi> ?
137 2012-01-07T23:44:02 <ThomasWaldmann> you could add a link to the flatland issue later
138 2012-01-07T23:44:55 <ThomasWaldmann> did you revalidate the output?
139 2012-01-07T23:45:17 <izibi> you mean in the flatland bug tracker?
140 2012-01-07T23:45:39 <izibi> yes. at least before changing the comment
141 2012-01-07T23:46:44 <izibi> "This document was successfully checked as HTML5!"
142 2012-01-07T23:48:31 <izibi> btw is there a reason why you didn't yet pull https://bitbucket.org/julianbrost/moin-2.0/changeset/959fc2a35823 / http://codereview.appspot.com/5504083/ into the main repository?
143 2012-01-07T23:48:39 <izibi> the current patch is based on the changes made there
144 2012-01-07T23:52:36 <ThomasWaldmann> well, i guess we'll check it after the current fixes. :)
145 2012-01-07T23:53:01 <ThomasWaldmann> if you want to help, merge main repo after your current task
146 2012-01-07T23:55:05 <izibi> ok, then i'll commit
147 2012-01-07T23:58:13 <izibi> done
148 2012-01-07T23:58:16 <izibi> https://bitbucket.org/julianbrost/moin-2.0/changeset/00895808c0af
149 2012-01-07T23:58:31 <izibi> and https://bitbucket.org/julianbrost/moin-2.0/changeset/cb16a89fcd49 (merge)
150 2012-01-07T23:59:51 *** raignarok
151
