Issue 4661057: alphabetic listing of global index, code modified
|
Created:
13 years, 5 months ago by sinha Modified:
13 years, 4 months ago Reviewers:
Reimar Bauer Visibility:
Public. |
Patch Set 1 #
Total comments: 5
Patch Set 2 : regex employed for filtering #
Total comments: 4
Patch Set 3 : frontend modified and some backend functions #
Total comments: 8
Patch Set 4 : item index link added beside each item name #
Total comments: 12
Patch Set 5 : change in view, modification in backend codes #
Total comments: 4
MessagesTotal messages: 14
some questions http://codereview.appspot.com/4661057/diff/1/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/4661057/diff/1/MoinMoin/apps/frontend/views.py#... MoinMoin/apps/frontend/views.py:489: prefix = request.values.get('prefix', None) What is the meaning of prefix? http://codereview.appspot.com/4661057/diff/1/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/1/MoinMoin/items/__init__.py#newco... MoinMoin/items/__init__.py:578: def flat_index(self, prefix=None): What is the meaning of prefix? http://codereview.appspot.com/4661057/diff/1/MoinMoin/items/__init__.py#newco... MoinMoin/items/__init__.py:581: prefix = (u"%s" % prefix, u"%s" % prefix.swapcase() ) why swapcase?
Sign in to reply to this message.
http://codereview.appspot.com/4661057/diff/1/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/1/MoinMoin/items/__init__.py#newco... MoinMoin/items/__init__.py:578: def flat_index(self, prefix=None): On 2011/06/29 06:36:30, Reimar Bauer wrote: > What is the meaning of prefix? that prefix is actually the alphabet. ex. to fetch list of items starting with letter 'A', so here 'A' will be prefix. http://codereview.appspot.com/4661057/diff/1/MoinMoin/items/__init__.py#newco... MoinMoin/items/__init__.py:581: prefix = (u"%s" % prefix, u"%s" % prefix.swapcase() ) On 2011/06/29 06:36:30, Reimar Bauer wrote: > why swapcase? because if ''A' is entered as input, then we should show the items starting with 'A' as well as 'a', so it will be case free.
Sign in to reply to this message.
questions http://codereview.appspot.com/4661057/diff/5001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/5001/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:578: def flat_index(self, startsWith=None): PEP8 for parameter http://codereview.appspot.com/4661057/diff/5001/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:580: import re why import at this line? http://codereview.appspot.com/4661057/diff/5001/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:583: item_re = u"^[%s].*" % startsWith is there a reason to add to the regex ^[%s].* ? besides the input is better readable. For POST it would be invisible. Do we expect that a user enters the query string manually or do we feed it only by a script? http://codereview.appspot.com/4661057/diff/5001/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:588: if u'/' not in relname and regex.match(relname)] match checks for a match only at the beginning of the string, while search checks for a match anywhere in the string The question is, do we need rather search or is match what we want. If you need match then ^ is not needed.
Sign in to reply to this message.
the proper name of the method is may be name_initial but it should then do nothing else see comments http://codereview.appspot.com/4661057/diff/8005/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/8005/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:580: item_name_re = u'' This name is misleading, because it is not the item_name regex It is likly the regex for startswith to find the names of items. http://codereview.appspot.com/4661057/diff/8005/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:588: new empty line, why? http://codereview.appspot.com/4661057/diff/8005/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:593: def first_letter(self, names=None): the first letter of a word is called "initial" may be a better name for this method is name_initial ? item.name_initial sounds better it should not be uppercased here. split this method in many parts 1.) get the initial of the items name 2.) uppercase, set, sort where you need that http://codereview.appspot.com/4661057/diff/8005/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:597: return set(letters) sets are a unordered collections of unique elements it is also faster if you have a smaller amount so first set then sort http://codereview.appspot.com/4661057/diff/8005/MoinMoin/templates/global_ind... File MoinMoin/templates/global_index.html (right): http://codereview.appspot.com/4661057/diff/8005/MoinMoin/templates/global_ind... MoinMoin/templates/global_index.html:23: {% for fullname, relname, contenttype in index[i:i+4] %} explain +4
Sign in to reply to this message.
http://codereview.appspot.com/4661057/diff/8005/MoinMoin/templates/global_ind... File MoinMoin/templates/global_index.html (right): http://codereview.appspot.com/4661057/diff/8005/MoinMoin/templates/global_ind... MoinMoin/templates/global_index.html:23: {% for fullname, relname, contenttype in index[i:i+4] %} On 2011/07/02 06:33:59, Reimar Bauer wrote: > explain +4 Showing 4 items in a single row.
Sign in to reply to this message.
see comment http://codereview.appspot.com/4661057/diff/8005/MoinMoin/templates/global_ind... File MoinMoin/templates/global_index.html (right): http://codereview.appspot.com/4661057/diff/8005/MoinMoin/templates/global_ind... MoinMoin/templates/global_index.html:23: {% for fullname, relname, contenttype in index[i:i+4] %} On 2011/07/02 06:53:25, sinha wrote: > On 2011/07/02 06:33:59, Reimar Bauer wrote: > > explain +4 > > Showing 4 items in a single row. ok, please make this a constant
Sign in to reply to this message.
ep http://codereview.appspot.com/4661057/diff/8005/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/8005/MoinMoin/items/__init__.py#ne... MoinMoin/items/__init__.py:578: def flat_index(self, startswith=None): see etherpad +182
Sign in to reply to this message.
Now links for item index have been added beside each item name, need to know whether this implementation is fine or not.
Sign in to reply to this message.
some questions some unspecific vars line 498 - 508 in views.py sounds like it should be simplified. http://codereview.appspot.com/4661057/diff/12002/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/4661057/diff/12002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:493: letters = item.name_initial(item.flat_index()) instead of letters is it initials http://codereview.appspot.com/4661057/diff/12002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:501: index2 = [] that name index2 is not specific http://codereview.appspot.com/4661057/diff/12002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:509: locale.setlocale(locale.LC_ALL, "") hmm, can we have a different locale.LC_ALL than the one defined in http://hg.moinmo.in/moin/2.0/file/tip/MoinMoin/config/default.py#l463 Do we need so set locale.setlocale(locale.LC_ALL, "") ? http://codereview.appspot.com/4661057/diff/12002/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:516: letters=letters, if it is initials then use that name, because letters is unspecific http://codereview.appspot.com/4661057/diff/12002/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/12002/MoinMoin/items/__init__.py#n... MoinMoin/items/__init__.py:592: letters = [(name[1][0]) you can name it initial here too It is singular because it is only 1 char http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... File MoinMoin/templates/global_index.html (right): http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... MoinMoin/templates/global_index.html:21: {% set item_per_row = 4 %} items_per_row http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... MoinMoin/templates/global_index.html:21: {% set item_per_row = 4 %} a question for now, because a fixed number did not scale good with large or small screens. Do you have a solution for that? can we do it with divs only? http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... MoinMoin/templates/global_index.html:35: {% endfor %} indenting change?
Sign in to reply to this message.
http://codereview.appspot.com/4661057/diff/12002/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/12002/MoinMoin/items/__init__.py#n... MoinMoin/items/__init__.py:592: letters = [(name[1][0]) On 2011/07/04 15:40:03, Reimar Bauer wrote: > you can name it initial here too > > It is singular because it is only 1 char you mean replacing letters by initial, but initial is list of characters so wouldn't "initials" be appropriate ? http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... File MoinMoin/templates/global_index.html (right): http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... MoinMoin/templates/global_index.html:21: {% set item_per_row = 4 %} On 2011/07/04 15:40:03, Reimar Bauer wrote: > a question for now, because a fixed number did not scale good with large or > small screens. Do you have a solution for that? > > can we do it with divs only? Yes, i did it initially with div, by using float: left property, by using that we can scale it depending on the available size, but the problem was that they weren't consistent, one row it will be 10, other it will be 5, with tables the look remains consistent, so i used it.
Sign in to reply to this message.
answered http://codereview.appspot.com/4661057/diff/12002/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/4661057/diff/12002/MoinMoin/items/__init__.py#n... MoinMoin/items/__init__.py:592: letters = [(name[1][0]) On 2011/07/04 18:52:12, sinha wrote: > On 2011/07/04 15:40:03, Reimar Bauer wrote: > > you can name it initial here too > > > > It is singular because it is only 1 char > > you mean replacing letters by initial, but initial is list of characters so > wouldn't "initials" be appropriate ? if it is a list one names it usually in the plural if it is a scalar you use the singular form so you are right, I have not looked carefully enough, my comment covered too much, sorry. http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... File MoinMoin/templates/global_index.html (right): http://codereview.appspot.com/4661057/diff/12002/MoinMoin/templates/global_in... MoinMoin/templates/global_index.html:21: {% set item_per_row = 4 %} On 2011/07/04 18:52:12, sinha wrote: > On 2011/07/04 15:40:03, Reimar Bauer wrote: > > a question for now, because a fixed number did not scale good with large or > > small screens. Do you have a solution for that? > > > > can we do it with divs only? > > Yes, i did it initially with div, by using float: left property, by using that > we can scale it depending on the available size, but the problem was that they > weren't consistent, one row it will be 10, other it will be 5, with tables the > look remains consistent, so i used it. that problem do you have also in tables if you have a columnsize of 10 the next 5 will end in the middle of the row. if there is a div also for the inner part with a fixed width it should work. Then we need to set that width only.
Sign in to reply to this message.
some small issues http://codereview.appspot.com/4661057/diff/17003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/4661057/diff/17003/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:500: index_more_links = [] this is the var for [..] flagging? link_to_item_indexes ? http://codereview.appspot.com/4661057/diff/17003/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:509: I miss the # TODO because of a better sort here
Sign in to reply to this message.
reg variable naming http://codereview.appspot.com/4661057/diff/17003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/4661057/diff/17003/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:500: index_more_links = [] On 2011/07/06 07:47:21, Reimar Bauer wrote: > this is the var for [..] flagging? > link_to_item_indexes ? this var has one more field than the original index, that extra field is link_to_item_indexes, thats why i named it as index_more_links .
Sign in to reply to this message.
ok http://codereview.appspot.com/4661057/diff/17003/MoinMoin/apps/frontend/views.py File MoinMoin/apps/frontend/views.py (right): http://codereview.appspot.com/4661057/diff/17003/MoinMoin/apps/frontend/views... MoinMoin/apps/frontend/views.py:500: index_more_links = [] On 2011/07/06 10:57:09, sinha wrote: > On 2011/07/06 07:47:21, Reimar Bauer wrote: > > this is the var for [..] flagging? > > link_to_item_indexes ? > > this var has one more field than the original index, that extra field is > link_to_item_indexes, thats why i named it as index_more_links . ok
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
