1 2013-07-15T01:00:51 <asingla> ThomasWaldmann: also updated https://codereview.appspot.com/10869045/
2 2013-07-15T05:54:06 *** rciorba
3 2013-07-15T06:34:37 *** rciorba
4 2013-07-15T06:45:10 *** rciorba
5 2013-07-15T07:44:13 <ananasova> moin
6 2013-07-15T08:27:05 <sharky93> moin
7 2013-07-15T08:31:01 <ananasova> waldi: updated admin email issue - please review https://codereview.appspot.com/11139044/
8 2013-07-15T08:54:05 *** greg_
9 2013-07-15T09:25:05 <sharky93> TheSheep: is it ok for you review https://codereview.appspot.com/11209044 as is ? since it is a bit odd, usually one cr has only one issue, but i've in a way used it to just list various diffs ..
10 2013-07-15T09:26:18 <TheSheep> sharky93: as long as the changes are logically grouped
11 2013-07-15T09:27:01 <TheSheep> sharky93: by the way, they are against which repo?
12 2013-07-15T09:37:33 <sharky93> TheSheep: this one @ https://bitbucket.org/sharky93/moin-2.0-bootstrap
13 2013-07-15T09:46:55 *** greg_
14 2013-07-15T09:56:02 <TheSheep> ok
15 2013-07-15T10:14:21 <ananasova> waldi: replied to some comments and updated the cr: https://codereview.appspot.com/11248043/
16 2013-07-15T11:12:58 *** dennda_
17 2013-07-15T11:36:13 *** greg_
18 2013-07-15T11:38:37 <sharky93> TheSheep: i was wondering before moving forward if we could commit the existing changes for the theme ?
19 2013-07-15T11:39:08 <TheSheep> it will take a moment more, sorry
20 2013-07-15T11:46:54 <sharky93> TheSheep: ok, so what should i proceed with?
21 2013-07-15T11:56:07 <TheSheep> sharky93: LESS and moin-specific classes?
22 2013-07-15T11:56:43 <ThomasWaldmann> TheSheep: btw, i talked to one guy who uses bootstrap classes directly
23 2013-07-15T11:56:56 <TheSheep> ThomasWaldmann: yeah, we do that too
24 2013-07-15T11:57:39 <ThomasWaldmann> he defended it by being easier (no need to fiddle with bootstrap dev setup), which also makes it easier to create new themes
25 2013-07-15T11:58:11 <TheSheep> ThomasWaldmann: yes, but you don't need to create new moin classes to make new themes
26 2013-07-15T11:58:37 <TheSheep> also, it does turn into </td><td> hell
27 2013-07-15T11:58:57 <ThomasWaldmann> but one needs the dev setup and "compile" bootstrap with the moin less code, right?
28 2013-07-15T12:00:10 <TheSheep> only if you need to modify the existing classes
29 2013-07-15T12:00:16 <TheSheep> or create new ones
30 2013-07-15T12:00:30 <TheSheep> you can still use bootstrap and moin classes directly in new themes
31 2013-07-15T12:00:41 <ThomasWaldmann> anyway, just wanted to throw in that information :)
32 2013-07-15T12:00:51 <TheSheep> thanks
33 2013-07-15T12:01:21 <ThomasWaldmann> ah, ok. just decide what's best for us. ;)
34 2013-07-15T12:05:25 <TheSheep> groan
35 2013-07-15T12:09:05 <sharky93> TheSheep: maybe proceed with some JS specific changes ?
36 2013-07-15T12:12:40 <TheSheep> ThomasWaldmann: I get whoosh.index.EmptyIndexError when trying to run moin index-create, is that normal?
37 2013-07-15T12:13:01 <ThomasWaldmann> starting from scratch?
38 2013-07-15T12:13:06 <TheSheep> yes
39 2013-07-15T12:13:09 <ThomasWaldmann> moin index-create -s -i
40 2013-07-15T12:13:33 <TheSheep> worked, thanks
41 2013-07-15T12:18:40 <TheSheep> sharky93: the breadcrumbs have an extra / at the end
42 2013-07-15T12:20:06 <sharky93> TheSheep: right. btw i was thinking, Maybe we should just commit this to the repo, and open the issue tracker ?
43 2013-07-15T12:20:20 <TheSheep> hmm
44 2013-07-15T12:20:24 <sharky93> ThomasWaldmann: too had suggested this earlier ..
45 2013-07-15T12:20:36 <TheSheep> that would certainly make it easier to review
46 2013-07-15T12:21:28 <TheSheep> by the way, the indentation in templates is all weird :/
47 2013-07-15T12:22:47 <sharky93> TheSheep: an example maybe?
48 2013-07-15T12:23:01 <sharky93> i might just be unaware of the norm :(
49 2013-07-15T12:24:09 * ThomasWaldmann would like to have that documented
50 2013-07-15T12:24:26 <ThomasWaldmann> (and, if possible, checked by a style checker)
51 2013-07-15T12:25:25 <ThomasWaldmann> but please no indentation change AND functional change in same changeset
52 2013-07-15T12:25:42 <TheSheep> sharky93: the search form tag, for example
53 2013-07-15T12:25:54 <TheSheep> sharky93: line 18 of layout.html
54 2013-07-15T12:26:30 <TheSheep> sharky93: also, the whole {% block layout %} is indented twice without a good reason
55 2013-07-15T12:27:00 <TheSheep> we don't have any guidelines at the moment
56 2013-07-15T12:27:12 <TheSheep> I need to write guidelines for my work too :(
57 2013-07-15T12:27:25 * ThomasWaldmann iirc used either 2 or 4 blanks, no tabs
58 2013-07-15T12:28:54 <TheSheep> look what I found https://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml
59 2013-07-15T12:32:41 <sharky93> TheSheep: hmm, "<div class="container-fluid">" was supposed to be the first indent for the block layout..
60 2013-07-15T12:33:14 <TheSheep> it's indented by 8 spaces
61 2013-07-15T12:34:53 <ThomasWaldmann> TheSheep: google-styleguide for css/html sounds reasonable. but in general be careful with them, iirc they use non-pep8 style for python.
62 2013-07-15T12:34:56 <TheSheep> I wonder if we should limit line length for html
63 2013-07-15T12:35:12 <TheSheep> ThomasWaldmann: yeah, I know, it's just an example and a starting point
64 2013-07-15T12:35:42 <ThomasWaldmann> same length as for python as far as I am concerned
65 2013-07-15T12:36:01 <ThomasWaldmann> == not strictly 80 if it just gets ugly by wrapping, but also not too long, like >120
66 2013-07-15T12:36:01 <TheSheep> then I recommend 2 spaces for indentation
67 2013-07-15T12:36:22 <TheSheep> as html is considerably more indented, and you can't really do much about it
68 2013-07-15T12:36:36 <sharky93> cool
69 2013-07-15T12:36:56 <TheSheep> but afaik we use 4 spaces everywhere now
70 2013-07-15T12:36:57 <ThomasWaldmann> sharky93: put that into rst docs of moin2
71 2013-07-15T12:37:12 <TheSheep> so not sure if it's worth changing
72 2013-07-15T12:38:12 <sharky93> we dont restrict length much too, buy maybe for the future ?
73 2013-07-15T12:38:47 <ThomasWaldmann> see above.
74 2013-07-15T12:39:00 <sharky93> ok
75 2013-07-15T12:39:39 <sharky93> TheSheep: so i make the commit ? and we work on with the issuetracker.. ?
76 2013-07-15T12:39:51 <TheSheep> sharky93: yeah
77 2013-07-15T12:40:06 <TheSheep> sharky93: I can also comment commits in bitbucket
78 2013-07-15T12:40:21 <TheSheep> I think
79 2013-07-15T12:40:52 <ThomasWaldmann> asingla: first review done
80 2013-07-15T12:41:14 <ThomasWaldmann> well, issue tracker is a bit more formal. noone will look at old commits, searching for todo.
81 2013-07-15T12:41:59 <TheSheep> yeah, comments for codign style, issues for todo
82 2013-07-15T12:47:06 <TheSheep> sharky93: one thing that I found useful is to break lines *inside* a tag when it has long attributes
83 2013-07-15T12:48:34 <TheSheep> http://paste.openstack.org/show/40414/
84 2013-07-15T12:48:36 <TheSheep> like this
85 2013-07-15T12:49:00 <sharky93> nice
86 2013-07-15T12:49:43 <TheSheep> also, note "" for quoting attributes, and '' for quoting python strings
87 2013-07-15T12:50:12 <TheSheep> argh, should be </a> of course
88 2013-07-15T12:51:16 * sharky93 should put all this on the etherpad
89 2013-07-15T12:51:42 <TheSheep> 14:36 < ThomasWaldmann> sharky93: put that into rst docs of moin2
90 2013-07-15T12:51:50 <sharky93> that too :)
91 2013-07-15T12:54:25 *** dave_largo
92 2013-07-15T12:56:02 * sharky93 looks up commit for mq
93 2013-07-15T12:56:46 <asingla> ThomasWaldmann: self.names can be none in the case there is no meta NAME.
94 2013-07-15T12:57:33 <asingla> maybe i can put a default value in .get so to make sure it does not return None.
95 2013-07-15T13:11:57 <ThomasWaldmann> asingla: maybe we should return a empty list? not sure, but sounds easier for callers.
96 2013-07-15T13:18:20 <ananasova> waldi: thanks for the comments :) updated here - https://codereview.appspot.com/11248043/
97 2013-07-15T13:22:03 * sharky93 lost all his mq changesets, will have to investigate what went wrong there -_-
98 2013-07-15T13:22:19 * sharky93 glad there;s still codereview. phew.
99 2013-07-15T13:23:54 <ananasova> sharky93: share the problem afterwords, we don't want to get traped too
100 2013-07-15T13:24:10 <sharky93> sure
101 2013-07-15T13:24:30 <sharky93> my heart nearly came out me mouth
102 2013-07-15T13:42:55 <rciorba> sharky93: you could version your patch queue as well, have a look at hg qcommit/qinit
103 2013-07-15T13:43:39 <rciorba> that way you'd have another repo for your queue that you could push to some other repo on bitbucket to have some backup and history
104 2013-07-15T13:55:53 <TheSheep> sharky93: I'm getting undefined error for 'utils'
105 2013-07-15T13:56:37 <TheSheep> sharky93: File "/home/sheep/dev/moin-2.0-bootstrap/MoinMoin/themes/basic/templates/show.html", line 235, in block "local_panel"
106 2013-07-15T14:22:15 <ThomasWaldmann> sharky93: that is also a good reason to try to not have too much locally, but published and pushed.
107 2013-07-15T14:23:13 <sharky93> rciorba: thanx, will have a look.
108 2013-07-15T14:24:06 <sharky93> TheSheep: ah, that got away. my bad. must have missed on putting that for codereview, i believe my working had that change. will fix it.
109 2013-07-15T14:44:01 <ananasova> waldi: left a comment on the issue https://codereview.appspot.com/11248043/
110 2013-07-15T15:00:05 *** rciorba
111 2013-07-15T15:02:39 <ananasova> and also updated the cr
112 2013-07-15T15:10:45 *** rciorba
113 2013-07-15T15:17:01 *** greg_
114 2013-07-15T15:23:58 * sharky93 pushed 11 commits to local repo :)
115 2013-07-15T15:24:11 <sharky93> TheSheep: you can pull now, it works
116 2013-07-15T15:34:09 <ThomasWaldmann> asingla: 2nd review done
117 2013-07-15T15:44:07 *** dreimark
118 2013-07-15T15:44:30 *** greg__
119 2013-07-15T15:51:43 <sharky93> TheSheep: currently active states for current link is enabled via JS, maybe we should try it using the template itself.
120 2013-07-15T15:51:58 * sharky93 wonders if its possible to tell the template which url has been called
121 2013-07-15T15:53:42 <asingla> bbl
122 2013-07-15T15:55:26 *** rciorba
123 2013-07-15T16:02:08 <sharky93> This is exactly what we want @ http://stackoverflow.com/questions/11665401/jinja-is-there-any-built-in-variable-to-get-current-html-page-name :)
124 2013-07-15T16:03:29 <sharky93> this one @ http://jinja.pocoo.org/docs/tricks/#highlighting-active-menu-items
125 2013-07-15T16:23:40 <ananasova> waldi: updated the issue - https://codereview.appspot.com/11248043/
126 2013-07-15T16:25:23 <waldi> ananasova: actually this was the wrong fix
127 2013-07-15T16:25:36 <waldi> you have to remove the comma if not necessary
128 2013-07-15T16:26:23 <waldi> hmm, i don't find it
129 2013-07-15T16:29:23 <ananasova> hmmm. some time ago thomas commented on an issue that i better use the following syntax [1, 2, 3, ] with a trailing comma, because it's less error prone.
130 2013-07-15T16:29:58 <ananasova> other moin code is following the same pattern
131 2013-07-15T16:31:13 <ananasova> check here https://codereview.appspot.com/11248043/diff2/8002:19001/MoinMoin/storage/middleware/validation.py and here https://codereview.appspot.com/11248043/diff2/19001:21001/MoinMoin/storage/middleware/validation.py
132 2013-07-15T16:31:24 <ananasova> waldi: ^
133 2013-07-15T16:37:18 <ananasova> waldi: please check the cr. removed the comma
134 2013-07-15T16:48:03 *** ReimarBauer
135 2013-07-15T16:56:24 *** dave_largo
136 2013-07-15T17:28:50 *** brunomartin
137 2013-07-15T17:52:05 *** greg__
138 2013-07-15T18:02:26 <asingla> ThomasWaldmann: updated https://codereview.appspot.com/10707048/
139 2013-07-15T18:17:51 *** rciorba
140 2013-07-15T19:23:59 *** dave_largo
141 2013-07-15T19:28:57 <asingla> ThomasWaldmann: more easier would be to add a argument 'names' instead of restrict_t. names_for_prefix = names or self.names ?
142 2013-07-15T19:29:18 <asingla> restrict_to_currentname*
143 2013-07-15T19:37:54 <ThomasWaldmann> can you rephrase that?
144 2013-07-15T19:39:02 <sharky93> re
145 2013-07-15T19:46:02 <asingla> ThomasWaldmann: see here .. https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py
146 2013-07-15T20:03:47 * ThomasWaldmann at dinner, but will look afterwards...
147 2013-07-15T20:47:09 <sharky93> ThomasWaldmann: why such declaration for item_name @ https://bitbucket.org/thomaswaldmann/moin-2.0/src/9c6013f454da2340be9586948c4b0aebd6f58e1d/MoinMoin/themes/__init__.py?at=default#cl-428 ?
148 2013-07-15T20:47:38 <sharky93> things may get confusing when you actually have such item_name
149 2013-07-15T20:49:31 <sharky93> TheSheep: ^
150 2013-07-15T20:59:51 *** dave_largo
151 2013-07-15T21:24:51 <ThomasWaldmann> sharky93: well, it was just to point to the issue if you in fact see that in a template
152 2013-07-15T21:29:21 <sharky93> ThomasWaldmann: i need to check if i;ve gotten some value from the handler, comparing the value would be a bad choice. :(
153 2013-07-15T21:32:41 <ThomasWaldmann> sharky93: you can change it to some shorter, visible value
154 2013-07-15T21:35:04 * ThomasWaldmann signed some gpg keys
155 2013-07-15T21:35:12 <sharky93> how can i be sure that it is not the actual item name ..?
156 2013-07-15T21:35:24 <sharky93> and the default thing
157 2013-07-15T21:36:09 <sharky93> s/and/but
158 2013-07-15T21:37:50 <ThomasWaldmann> we'll disallow item names starting with @, so use @NONAME@ or something
159 2013-07-15T21:41:24 <spy_> ThomasWaldmann: dreimark: moin, have you seen an updated cr: https://codereview.appspot.com/10761044
160 2013-07-15T21:45:54 * ThomasWaldmann looks
161 2013-07-15T22:08:46 <ThomasWaldmann> asingla: can we already create an Item with no name (Item.create) ?
162 2013-07-15T22:09:28 * ThomasWaldmann reviews the comment system, it is needed there.
163 2013-07-15T22:49:24 <asingla> ThomasWaldmann: no, but I added a fix for it.
164 2013-07-15T22:50:49 <asingla> https://codereview.appspot.com/10519045/ this one deals with it.
165 2013-07-15T23:02:53 *** rciorba
166 2013-07-15T23:05:38 *** ronny
167
