Issue 99560043: Passed function as parameter(gives error) (Closed)
|
Created:
9 years, 11 months ago by sksaurabhkathpalia Modified:
9 years, 11 months ago Visibility:
Public. |
DescriptionPassed function as parameter(gives error)
Patch Set 1 #
Total comments: 5
Patch Set 2 : removed a small error #
Total comments: 3
Patch Set 3 : Now summary is being shown #
Total comments: 2
Patch Set 4 : imported summary key #Patch Set 5 : used the first name of the name list #Patch Set 6 : used .format instead of concatenation #
Total comments: 2
Patch Set 7 : Made the code more readable #
Total comments: 1
Patch Set 8 : some minor changes #
Total comments: 4
Patch Set 9 : Made the code more readable #
Total comments: 2
Patch Set 10 : Removed pep-8 error #
Total comments: 3
Patch Set 11 : Removed pep-8 error #Patch Set 12 : Made some minor changes #
Total comments: 2
Patch Set 13 : Added commas #MessagesTotal messages: 17
https://codereview.appspot.com/99560043/diff/1/MoinMoin/forms.py File MoinMoin/forms.py (right): https://codereview.appspot.com/99560043/diff/1/MoinMoin/forms.py#newcode355 MoinMoin/forms.py:355: key = cls.properties['label_meta_key'] for what do you still need label_meta_key? https://codereview.appspot.com/99560043/diff/1/MoinMoin/forms.py#newcode364 MoinMoin/forms.py:364: return rev.meta[ITEMID] + '(' + rev.meta[u'summary'] +')' why don't you define this in ticket.py? also, giving the full itemid and directly append the summary likely will look ugly. think of a prettier format (or just see what I told on irc yday). https://codereview.appspot.com/99560043/diff/1/MoinMoin/forms.py#newcode366 MoinMoin/forms.py:366: choices = [(rev.meta[ITEMID], label_getter(rev)) for rev in revs] where is label_getter defined? you should handle this in precisely the same way as you are dealing with label_meta_key right now (as a replacement of it, not additionally). https://codereview.appspot.com/99560043/diff/1/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/1/MoinMoin/items/ticket.py#newco... MoinMoin/items/ticket.py:38: OptionalTicketReference = Reference.to(TICKET_QUERY).using(optional=True).with_properties(label_getter = itemid_short_summary) obey pep8 (no spaces around "=" if it is in a function call). if you use the name itemid_summary here, maybe also just define it right above that line. https://codereview.appspot.com/99560043/diff/1/MoinMoin/items/ticket.py#newco... MoinMoin/items/ticket.py:39: OptionalUserReference = Reference.to(USER_QUERY).using(optional=True).with_properties(empty_label='(Nobody)', label_getter=get_label) for users, displaying the name as label makes sense. so you need a getter that gets the name.
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/20001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/20001/MoinMoin/items/ticket.py#n... MoinMoin/items/ticket.py:38: OptionalTicketReference = Reference.to(TICKET_QUERY).using(optional=True).with_properties(label_getter = itemid_short_summary) label_getter=itemid_short_summary you don't need spaces around =, if it's a function call https://codereview.appspot.com/99560043/diff/20001/MoinMoin/items/ticket.py#n... MoinMoin/items/ticket.py:38: OptionalTicketReference = Reference.to(TICKET_QUERY).using(optional=True).with_properties(label_getter = itemid_short_summary) itemid_short_summary is not defined yet. https://codereview.appspot.com/99560043/diff/20001/MoinMoin/items/ticket.py#n... MoinMoin/items/ticket.py:39: OptionalUserReference = Reference.to(USER_QUERY).using(optional=True).with_properties(empty_label='(Nobody)', label_getter=get_label, label_meta_key = NAME) in get_label, i guess is not defined, you need to define a function, which you will pass later.
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/40001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/40001/MoinMoin/items/ticket.py#n... MoinMoin/items/ticket.py:40: return rev.meta[ITEMID][:4] + ' ( ' + rev.meta[u'summary'][:50] +' )' please use a .format() instead of string concatination: return '{itemid}' ({summary}).format(itemid=rev.meta[ITEMID][:4], summary=rev.meta[u'summary'][:50]) )
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/40001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/40001/MoinMoin/items/ticket.py#n... MoinMoin/items/ticket.py:40: return rev.meta[ITEMID][:4] + ' ( ' + rev.meta[u'summary'][:50] +' )' On 2014/05/28 17:15:19, dimazest wrote: > please use a .format() instead of string concatination: > > return '{itemid}' ({summary}).format(itemid=rev.meta[ITEMID][:4], > summary=rev.meta[u'summary'][:50]) > > ) Done.
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/100001/MoinMoin/forms.py File MoinMoin/forms.py (right): https://codereview.appspot.com/99560043/diff/100001/MoinMoin/forms.py#newcode354 MoinMoin/forms.py:354: choices = [(rev.meta[ITEMID], cls.properties['label_getter'](rev)) for rev in revs] label_getter = cls.properties['label_getter'] use it later, this will make the code more readable https://codereview.appspot.com/99560043/diff/100001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/100001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:45: OptionalTicketReference = Reference.to(TICKET_QUERY).using(optional=True).with_properties(label_getter=get_itemid_short_summary) i guess the line is too long, is it possible to split it to several, similarly to #181?
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/120001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/120001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:47: ) ideally it should be: OptionalTicketReference = Reference.to(TICKET_QUERY).using(optional=True).with_properties( label_getter=get_itemid_short_summary )
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:45: OptionalTicketReference = \ \ is unnecessary. OptionalTicketReference = Reference.to ...
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:45: OptionalTicketReference = \ On 2014/05/28 21:40:26, dimazest wrote: > \ is unnecessary. > > OptionalTicketReference = Reference.to ... This is python syntax. I mean \ is required. If this is to be removed then it should be like this OptionalTicketReference = Reference.to(TICKET_QUERY).using(optional=True).with_properties( label_getter=get_itemid_short_summary )
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:45: OptionalTicketReference = \ what about, thought it's also ugly. OptionalTicketReference = Reference.to( TICKET_QUERY ).using( optional=True ).with_properties( label_getter=get_itemid_short_summary, ) or OptionalTicketReference = ( Reference .to(TICKET_QUERY) .using(optional=True) .with_properties(label_getter=get_itemid_short_summary) ) On 2014/05/29 02:42:38, sksaurabhkathpalia wrote: > On 2014/05/28 21:40:26, dimazest wrote: > > \ is unnecessary. > > > > OptionalTicketReference = Reference.to ... > > This is python syntax. I mean \ is required. > If this is to be removed then it should be like this > > OptionalTicketReference = > Reference.to(TICKET_QUERY).using(optional=True).with_properties( > label_getter=get_itemid_short_summary > )
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/140001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:45: OptionalTicketReference = \ On 2014/05/29 09:42:29, dimazest wrote: > what about, thought it's also ugly. > > OptionalTicketReference = Reference.to( > TICKET_QUERY > ).using( > optional=True > ).with_properties( > label_getter=get_itemid_short_summary, > ) > > or > > OptionalTicketReference = ( > Reference > .to(TICKET_QUERY) > .using(optional=True) > .with_properties(label_getter=get_itemid_short_summary) > ) > > > On 2014/05/29 02:42:38, sksaurabhkathpalia wrote: > > On 2014/05/28 21:40:26, dimazest wrote: > > > \ is unnecessary. > > > > > > OptionalTicketReference = Reference.to ... > > > > This is python syntax. I mean \ is required. > > If this is to be removed then it should be like this > > > > OptionalTicketReference = > > Reference.to(TICKET_QUERY).using(optional=True).with_properties( > > label_getter=get_itemid_short_summary > > ) > Done.
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/160001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/160001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:39: def get_itemid_short_summary(rev): ================================== FAILURES =================================== _____________________ PEP8-check(ignoring E124 E125 E501) _____________________ D:\Bitbucket\AAsaurabh\MoinMoin\items\ticket.py:39:1: E302 expected 2 blank lines, found 1 def get_itemid_short_summary(rev): ^ D:\Bitbucket\AAsaurabh\MoinMoin\items\ticket.py:42:1: E302 expected 2 blank lines, found 1 def get_name(rev): ^
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/160001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/160001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:39: def get_itemid_short_summary(rev): On 2014/05/29 14:13:50, RogerHaase wrote: > ================================== FAILURES =================================== > _____________________ PEP8-check(ignoring E124 E125 E501) _____________________ > D:\Bitbucket\AAsaurabh\MoinMoin\items\ticket.py:39:1: E302 expected 2 blank > lines, found 1 > def get_itemid_short_summary(rev): > ^ > D:\Bitbucket\AAsaurabh\MoinMoin\items\ticket.py:42:1: E302 expected 2 blank > lines, found 1 > def get_name(rev): > ^ Done.
Sign in to reply to this message.
code looks ok, pep8? https://codereview.appspot.com/99560043/diff/170001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/170001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:61: ) is our pep8 checker happy with that?
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/170001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/170001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:61: ) actually, i'm not sure what would be the best layout. another alternatives are OptionalTicketReference = ( Reference .to(...) .using(..) .with_properties(...) ) or putting everything on one line On 2014/05/29 16:29:26, Thomas.J.Waldmann wrote: > is our pep8 checker happy with that?
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/170001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/170001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:61: ) On 2014/05/29 16:29:26, Thomas.J.Waldmann wrote: > is our pep8 checker happy with that? Done.
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/210001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/210001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:51: optional=True i this cases I usually add "," in the end: OptionalTicketReference = Reference.to( TICKET_QUERY, ) the reason is, if arguments are added, then the diff will not contain the additional comma which is not relevant to the change
Sign in to reply to this message.
https://codereview.appspot.com/99560043/diff/210001/MoinMoin/items/ticket.py File MoinMoin/items/ticket.py (right): https://codereview.appspot.com/99560043/diff/210001/MoinMoin/items/ticket.py#... MoinMoin/items/ticket.py:51: optional=True On 2014/05/30 05:54:31, dimazest wrote: > i this cases I usually add "," in the end: > > OptionalTicketReference = Reference.to( > TICKET_QUERY, > ) > > the reason is, if arguments are added, then the diff will not contain the > additional comma which is not relevant to the change Done.
Sign in to reply to this message.
|
