Issue 10519045: Added basic composite name support
|
Created:
10 years, 11 months ago by ashu1461 Modified:
10 years, 10 months ago Reviewers:
thomas.j.waldmann Visibility:
Public. |
DescriptionAdded basic composite name support
Patch Set 1 : Added basic comp name support #
Total comments: 23
Patch Set 2 : #
Total comments: 1
Patch Set 3 : Testing done. #
Total comments: 3
Patch Set 4 : #
Total comments: 1
MessagesTotal messages: 8
https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:113: self.meta[item.fqname.field] = [item.fqname.value] if item.fqname.field == NAME_EXACT else item.fqname.value the new name fqname is now a namedtuple (namespace, field, value)
Sign in to reply to this message.
https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:113: self.meta[item.fqname.field] = [item.fqname.value] if item.fqname.field == NAME_EXACT else item.fqname.value the line above is not correct in general, you are only checking NAME_EXACT vs. anything else. it is also not clear that this operation is wanted anyway. It makes sense if one creates a new item via name. but what if e.g. a tag is given? https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:119: self.fqname = fqname did you do a global review for access to .name to be sure you found and changed all places to .fqname? https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:243: #Composite Name -> Namespace + field + value if you put a comment, leave a blank after the #. (and, if the comment is to the right of a statement put 2 blanks before the #.) that said, the comment is pretty useless here as it does not contain more information than what's visible in the line below anyway. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:285: namespace, field, value = split_fqname(name) why does this call not directly return the named tuple? https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:286: field = NAME_EXACT if not field else field that could also get handled there. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:316: return get_fqname(self.fqname.value, u'' if self.fqname.field == NAME_EXACT else self.fqname.field, self.fqname.namespace) hmm, you could maybe subclass namedtuple, add a __unicode__ method there and do this there. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:324: return self.meta.get(NAME) why not ONLY use the last line in every case? https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:336: kill_keys = [ # shall not get copied from old rev to new rev did you read this comment? ^^ https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:513: meta[self.fqname.field] = self.fqname.value the last line is not correct in any case. e.g. if field is NAME_EXACT, you overwrite the stuff that you had just built in the lines above with a incorrect value. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:602: fqname = CompositeName(namespace, field, value) see comment above https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:761: return redirect(url_for_item(namespace=self.fqname.namespace, field=self.fqname.field, item_name=self.fqname.value)) think about a way so that such stuff is less verbose. (could be a method of fqname that returns a dict with such key/values that can be exploded with **)
Sign in to reply to this message.
https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:113: self.meta[item.fqname.field] = [item.fqname.value] if item.fqname.field == NAME_EXACT else item.fqname.value On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > the line above is not correct in general, you are only checking NAME_EXACT vs. > anything else. > > it is also not clear that this operation is wanted anyway. It makes sense if one > creates a new item via name. but what if e.g. a tag is given? i think maybe do a bit more checks here. if field ITEMID: #enabling item creation on the basis on itemid. save as meta[field] = value else if field NAME_EXACT: save as meta[field] = value else dont save. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:119: self.fqname = fqname On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > did you do a global review for access to .name to be sure you found and changed > all places to .fqname? I have defined name as a property, so access by .name wont be a problem. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:243: #Composite Name -> Namespace + field + value On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > if you put a comment, leave a blank after the #. > > (and, if the comment is to the right of a statement put 2 blanks before the #.) > > that said, the comment is pretty useless here as it does not contain more > information than what's visible in the line below anyway. ok fine. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:286: field = NAME_EXACT if not field else field On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > that could also get handled there. you mean to handle this stuff in interwiki module ? we would have to modify some other interwiki functions/tests though then, but it wont be much of a problem. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:324: return self.meta.get(NAME) On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > why not ONLY use the last line in every case? currently we cannot. self.name was a unicode, so many funcs like get_mixed_index, subitems_prefix etc dont work if self.name is a list. Firstly we have to make sure that there is no problem anywhere dealing with name being a list. i am working on it. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:336: kill_keys = [ # shall not get copied from old rev to new rev On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > did you read this comment? ^^ REVID killing can be avoided, but we need ITEMID in meta at this stage in case we are creating a item by ITEMID
Sign in to reply to this message.
https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:113: self.meta[item.fqname.field] = [item.fqname.value] if item.fqname.field == NAME_EXACT else item.fqname.value *in case of NAME_EXACT meta[field] = [value]
Sign in to reply to this message.
you didn't react to all comments, so i suggest you review them again. a reaction can either be another comment or a code change. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:113: self.meta[item.fqname.field] = [item.fqname.value] if item.fqname.field == NAME_EXACT else item.fqname.value can you classify the field keys, put them into different sets and just use "else" for the invalid case (but otherwise a set member check)? if the sets are rather small, you can also use lists. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:286: field = NAME_EXACT if not field else field On 2013/06/26 09:00:30, ashu1461 wrote: > On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > > that could also get handled there. > > you mean to handle this stuff in interwiki module ? > we would have to modify some other interwiki functions/tests though then, but it > wont be much of a problem. shouldn't be much. create clean changesets, don't mix a lot of stuff into 1 cs. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:336: kill_keys = [ # shall not get copied from old rev to new rev On 2013/06/26 09:00:30, ashu1461 wrote: > On 2013/06/25 21:42:25, Thomas.J.Waldmann wrote: > > did you read this comment? ^^ > REVID killing can be avoided, > but we need ITEMID in meta at this stage in case we are creating a item by > ITEMID OK, then don't remove any stuff if you have no reason for that. For itemid creation: ok. we need to check validation for that. https://codereview.appspot.com/10519045/diff/7001/MoinMoin/items/__init__.py#... MoinMoin/items/__init__.py:513: meta[self.fqname.field] = self.fqname.value no comment?
Sign in to reply to this message.
https://codereview.appspot.com/10519045/diff/21001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/21001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:116: self.meta[NAME] = [item.fqname.value] this is pretty much equivalent to what you had. read again my last review...
Sign in to reply to this message.
https://codereview.appspot.com/10519045/diff/25001/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/10519045/diff/25001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:119: ITEMID, REVID, TAGS, USERID, ITEMLINKS, ITEMTRANSCLUSIONS, NAME_EXACT end such lists with a comma it is useful / more pretty diff when more stuff gets added later. if you use a list (that will be searched sequentially), put the more likely candidates at the beginning. https://codereview.appspot.com/10519045/diff/25001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:123: ITEMID, REVID, NAME_EXACT, USERID why USERID? check how it is used... https://codereview.appspot.com/10519045/diff/25001/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/25001/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:315: def name(self): docstring missing
Sign in to reply to this message.
https://codereview.appspot.com/10519045/diff/37002/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): https://codereview.appspot.com/10519045/diff/37002/MoinMoin/items/__init__.py... MoinMoin/items/__init__.py:317: returns the item name well, as items may have 0, 1 or more names, what is "the item name"? also, considering the 0 case, code below looks like it would crash.
Sign in to reply to this message.
|
