major types are replaced by contenttype groups

some questions and screenshot at diary page http://moinmo.in/AkashSinha/Gsoc2011Diary/2011-07-09

[Reimar Bauer, 2011-07-10 07:33:11]

some comments

http://codereview.appspot.com/4686045/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/4686045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:496: all_contenttypes = ["application", "audio",
"image", "text", "video"]
name it major_contenttypes

http://codereview.appspot.com/4686045/diff/1/MoinMoin/apps/frontend/views.py#...
MoinMoin/apps/frontend/views.py:498: posted_fields = request.values.to_dict()
give it a more specific name

http://codereview.appspot.com/4686045/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/4686045/diff/1/MoinMoin/items/__init__.py#newco...
MoinMoin/items/__init__.py:586: if u'/' not in relname and
(contenttype.split('/', 1)[0] in contenttypes) and
relname.startswith(startswith)]
line break

http://codereview.appspot.com/4686045/diff/1/MoinMoin/templates/global_index....
File MoinMoin/templates/global_index.html (left):

http://codereview.appspot.com/4686045/diff/1/MoinMoin/templates/global_index....
MoinMoin/templates/global_index.html:3: {% block item %}
see wiki page

http://codereview.appspot.com/4686045/diff/1/MoinMoin/templates/global_index....
File MoinMoin/templates/global_index.html (right):

http://codereview.appspot.com/4686045/diff/1/MoinMoin/templates/global_index....
MoinMoin/templates/global_index.html:4: <div class="moin-select-contenttypes">
it is major_contenttype

http://codereview.appspot.com/4686045/diff/1/MoinMoin/themes/modernized/stati...
File MoinMoin/themes/modernized/static/css/common.css (right):

http://codereview.appspot.com/4686045/diff/1/MoinMoin/themes/modernized/stati...
MoinMoin/themes/modernized/static/css/common.css:266: 
empty line

[Reimar Bauer, 2011-07-12 07:58:51]

some comments, docstring missing and look at unit tests please

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:578: def flat_index(self, startswith=None,
selected_groups=None):
docstring and parameter description

unittest checked for that change? or needs unittest?

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:680: def contenttype_group(self, contenttype):
docstring and param description

add a unit test for that too

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:684: group = groups[0]
linebreak in listcomprehensions, that makes it better readable.

it looks a bit complicated because you take only the first element of the result
later.

in that case i am not sure if we save really time if we break the first loop. 
it may be better readable.

[ThomasWaldmann, 2011-07-12 08:27:37]

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:497: posted_fields = request.values.to_dict()
"post"ed? request.values has post form fields PLUS get url args. if you just
want posted stuff, use request.form.

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:500: index = item.flat_index(startswith,
selected_groups)
is "startswith" an item name or can it be something else?

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:503: contenttype_groups =
nonexistent_item.contenttype_groups
please move contenttype_groups to another place, so you do not need such hacks
just to access it.

i suggest putting it into MoinMoin/config/__init__.py, and access it from there
(config.contenttype_groups) from all places needing it.

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:508: initials = list(set(initials))
wrong order. if you want to use set, to remove duplicates, you should do that
early, before doing other operations, so that the other operations get a shorter
list to operate on.

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:509: initials.sort()
consider using sorted()

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:514: selected_groups = [gname for gname,
ctlabels in contenttype_groups]
do this some lines above, so all operations on selected_groups are at one place

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (right):

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:580: nonexistent_item = NonExistent(self)
see comment in other file

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:588: and
(nonexistent_item.contenttype_group(contenttype) in selected_groups)
maybe you should first (before the loop) compute what contenttypes we have
selected (expand the group name to a set of specific content types).

then you can just say "contenttype in selected_contenttypes" here, much faster.

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/items/__init__.py#n...
MoinMoin/items/__init__.py:680: def contenttype_group(self, contenttype):
see comment above, this seems too complicated.

[sinha, 2011-07-12 09:05:04]

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:497: posted_fields = request.values.to_dict()
On 2011/07/12 08:27:37, ThomasWaldmann wrote:
> "post"ed? request.values has post form fields PLUS get url args. if you just
> want posted stuff, use request.form.

i wanted here both ( posted as well the url args), so i guess this name is a
little confusing one.

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:500: index = item.flat_index(startswith,
selected_groups)
On 2011/07/12 08:27:37, ThomasWaldmann wrote:
> is "startswith" an item name or can it be something else?

it will be any single character, which is used to filter items whose name starts
with this character.

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:503: contenttype_groups =
nonexistent_item.contenttype_groups
On 2011/07/12 08:27:37, ThomasWaldmann wrote:
> please move contenttype_groups to another place, so you do not need such hacks
> just to access it.
> 
> i suggest putting it into MoinMoin/config/__init__.py, and access it from
there
> (config.contenttype_groups) from all places needing it.

this is also used inside some of the Item class methods, do you want me to do
changes there also ?

[sinha, 2011-07-12 13:36:53]

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/4686045/diff/11001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:508: initials = list(set(initials))
On 2011/07/12 08:27:37, ThomasWaldmann wrote:
> wrong order. if you want to use set, to remove duplicates, you should do that
> early, before doing other operations, so that the other operations get a
shorter
> list to operate on.

The only reason set is applied here is to remove the repeated chars, and chars
will only be repeated after the above step, otherwise i wouldn't be using the
set at all.

Ex: we have [b,a,D,A]
after doing upper, i get [b,A,D,A]
so after this step only, set will make sense.

[sinha, 2011-07-12 14:11:47]

change in code for filtering by contenttype, docstring and unittest added for
flat_index function

I will move the contenttype_groups data-strcuture to config after commiting this
one.

[ThomasWaldmann, 2011-07-12 14:35:33]

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/templates/global_in...
File MoinMoin/templates/global_index.html (right):

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/templates/global_in...
MoinMoin/templates/global_index.html:16: <label for="{{ gname }}">{{
gname|replace("items", "") }}</label>
i think you'll want i18n here, right?

maybe try without replace tricks.

[sinha, 2011-07-12 14:39:11]

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/templates/global_in...
File MoinMoin/templates/global_index.html (right):

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/templates/global_in...
MoinMoin/templates/global_index.html:16: <label for="{{ gname }}">{{
gname|replace("items", "") }}</label>
On 2011/07/12 14:35:33, ThomasWaldmann wrote:
> i think you'll want i18n here, right?
> 
> maybe try without replace tricks.

i guess all these names are written in plain english, maybe due to environment
we get unicodes.
anyways i will try with substring methods.

[Reimar Bauer, 2011-07-12 19:00:34]

small comments

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:513: detailed_index = sorted(detailed_index)
look at the key functions, if you can use it e.g. with str.lower 

see http://wiki.python.org/moin/HowTo/Sorting/

[sinha, 2011-07-12 19:15:38]

replied.

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:513: detailed_index = sorted(detailed_index)
On 2011/07/12 19:00:34, Reimar Bauer wrote:
> look at the key functions, if you can use it e.g. with str.lower 
> 
> see http://wiki.python.org/moin/HowTo/Sorting/

i tried but here list elements are tuple, so that 
]$ sorted(detailed_index, key=str.lower) is throwing errors, 
i am thinking of using lambda as 
]$ sorted(detailed_index, key=lambda item_info: item_info[1])

but here i may have to add the cmp function also, otherwise it is not helping
any better.

[Reimar Bauer, 2011-07-12 20:13:49]

use a different list and get that index

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

http://codereview.appspot.com/4686045/diff/21001/MoinMoin/apps/frontend/views...
MoinMoin/apps/frontend/views.py:513: detailed_index = sorted(detailed_index)
On 2011/07/12 19:15:38, sinha wrote:
> On 2011/07/12 19:00:34, Reimar Bauer wrote:
> > look at the key functions, if you can use it e.g. with str.lower 
> > 
> > see http://wiki.python.org/moin/HowTo/Sorting/
> 
> i tried but here list elements are tuple, so that 
> ]$ sorted(detailed_index, key=str.lower) is throwing errors, 
> i am thinking of using lambda as 
> ]$ sorted(detailed_index, key=lambda item_info: item_info[1])
> 
> but here i may have to add the cmp function also, otherwise it is not helping
> any better.

you can create a different list in lower case and get how that is sorted and
apply that index to your data