Issue 10707048: Subitems views fixed.
|
Created:
10 years, 10 months ago by ashu1461 Modified:
10 years, 10 months ago Reviewers:
thomas.j.waldmann Visibility:
Public. |
Description1) Subitems views fixed.
2) A little change in the item trail, to render urls correctly.
Patch Set 1 : #
Total comments: 9
Patch Set 2 : fixed previous mistakes .. #
Total comments: 7
Patch Set 3 : Updated names func .. #
Total comments: 4
MessagesTotal messages: 10
https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:120: self.meta[item.fqname.field] = item.fqname.value On new item creation where earlier meta NAME used to appear, NAME_EXACT appears, NAME_EXACT being the field, if we save the value in the name meta here, it can save further modification of existing code. https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:340: return self.meta.get(NAME) we cannot impose the name property to be a list at once, the whole code has to be changed step by step to do this. let page_name be a property that is a list, the new code can use page_name, which later can be changed to name itself. https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:623: if fullname not in added_fullnames: Scenario item A [name: (foo, bar) and itemid (ITEMID)] foo has subitems foo/1, foo/2 bar has subitems bar/1, bar/2 fullname should be used as comparator between subitems of same item. relname would be same for baa/1 and foo/1 (i.e 1 which would create the problem.)
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:327: except: # except AnyError don't use except: if you can more specifically catch an error, it triggers warnings in linters. in more complex cases than here, it might also suppress all sorts of issues (even the ones you did not want to catch). https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:331: def page_name(self): bad method name. using a singular name if a list of items is meant is a bad idea. iirc we already have such methods, see the storage items, maybe use similar / same naming convention. https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:340: return self.meta.get(NAME) code mostly duplicated from name method. https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:560: def subitems_prefix(self): if you are going to use a list: bad method name, see above. https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:564: return u'' huh? sometimes returning a list, sometimes a single string?
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/22001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/22001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:772: state = dict(name=self.name, itemid=self.meta.get(ITEMID)) please ignore this one ..
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:769: state = dict(name=self.name, itemid=self.meta.get(ITEMID)) please ignore this change ..
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/5001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:112: if self.item: sometimes you are using item, sometimes self.item. suggest you use item. also, insert a line "fqname = item.fqname" below this, so you can reduce the code duplication in the next few lines. https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:320: returns the page name if known else extract name from meta. doesn't match the code of THIS method any more also s/page/item/ https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:324: except (TypeError, IndexError): why can it be TypeError? https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:330: This is the name, just the format is List. hmm, "the name" sounds like exactly one. just say "returns a list of 0..n names of the item"? https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:336: return self.meta.get(NAME) while thinking about this, I had a slightly different idea. returning just a single name for first case (if-block) and the complete list in the other case (else-block) is rather unpretty and inconsistent. how about getting the list of names from meta in any case, and then, IF we know we are dealing with one specific name, move it to position 0 of the list (== remove it from list, insert it at position 0 again). so upper layers of the code can always use n[0] if they want one name and n if they want to have a list of all names. https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:557: return [name + u'/' if name else u'' for name in self.names] if self.names else [] "if self.names else []" as long as self.names is a list always, this looks rather superfluous, right? The left part would also just give a empty list. About "if name else u''" - what kind of "falsy" names do you expect and where do they come from? https://codereview.appspot.com/10707048/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:567: def get_subitem_revs(self): just noticed a naming inconsistency: subitem_revs subitems_prefixes
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:324: except IndexError: earlier TypeError because, self.names could be none .. now no suck possibility.
Sign in to reply to this message.
> now no suck possibility. haha. :P
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:561: names = self.names[0:1] if self.fqname.field == NAME_EXACT else self.names maybe this is rather a question of what the caller wants and intends to do with the results (and not so much whether we identified the item by name or itemid). whether he only wants the prefixes for the current name or any. maybe use a flag argument? restrict_to_currentname=False argument maybe?
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:561: names = self.names[0:1] if self.fqname.field == NAME_EXACT else self.names would be easier to use argument 'names' instead of 'restrict_to_currentname' if a user wants to obtain prefixes for particular names (whether current name or any), he can pass it through the names argument. Using restrict_to_currentname would lead to code duplication as each time the function is called we would have to pre-evaluate whether the value is False(mainly if the field is NAME_EXACT)/True(mainly if the filed is ITEMID).
Sign in to reply to this message.
https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10707048/diff/32001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:561: names = self.names[0:1] if self.fqname.field == NAME_EXACT else self.names hmm, ok, keep it for now. i just found the magically different behaviour strange (and it is also not documented).
Sign in to reply to this message.
|
