Issue 93570043: Added fqname suport to notifications module (Closed)
|
Created:
9 years, 11 months ago by sksaurabhkathpalia Modified:
9 years, 11 months ago Reviewers:
thomas.j.waldmann, dimazest Visibility:
Public. |
DescriptionAdded fqname suport to notifications module
Patch Set 1 #Patch Set 2 : Changed the item urls(used fqname directly in urls) #Patch Set 3 : Changed one test and done some other minor changes also (Now one unit test jfu_server is failing) #
Total comments: 1
Patch Set 4 : Removed pep-8 errors #Patch Set 5 : Now all unit tests are passing #
Total comments: 11
Patch Set 6 : Done a minor change in notifications.py #
Total comments: 7
Patch Set 7 : Removed code duplication #
Total comments: 2
Patch Set 8 : Removed small errors #
Total comments: 4
Patch Set 9 : Changed label to labels and field to label #Patch Set 10 : Added item_url in message #
Total comments: 15
Patch Set 11 : Used fqname.value instead of fqname[2] and also changed the parameters to signal item_displayed #
Total comments: 2
Patch Set 12 : Done a minor change #Patch Set 13 : Used fqname.value and fqname.field directly #
Total comments: 7
Patch Set 14 : Used L_ in labels also #Patch Set 15 : Used unicode(fqname) instead of label #
Total comments: 4
Patch Set 16 : Used fqname instead of label and removed item_name from subject and message #Patch Set 17 : Removed "with item" #Patch Set 18 : Full tested the code (now mail is being sent) did a small change in subscriptions.py #
MessagesTotal messages: 37
https://codereview.appspot.com/93570043/diff/40001/MoinMoin/forms.py File MoinMoin/forms.py (right): https://codereview.appspot.com/93570043/diff/40001/MoinMoin/forms.py#newcode361 MoinMoin/forms.py:361: This was just a pep8 error
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/apps/frontend/vie... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/apps/frontend/vie... MoinMoin/apps/frontend/views.py:829: except AccessDenied: this is not really related to you change, but why not to narrow down the try ... except clause to the line that actually throws 403. then the code will become try: ... exxcept AccessDenied: abort(403) else: continue https://codereview.appspot.com/93570043/diff/80001/MoinMoin/signalling/log.py File MoinMoin/signalling/log.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/signalling/log.py... MoinMoin/signalling/log.py:25: logging.info(u"item {0}:{1} modified".format(wiki_name, fqname[2])) wouldn't it be better to use logging like this: logging.info(u"item %s:%s modified", wiki_name, fqname[2]) then log aggregators, such as sentry, could group such messages. https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... MoinMoin/util/notifications.py:84: kw = dict(item_name=self.fqname[2], wiki_name=self.wiki_name, user_name=flaskg.user.name0) would it make sense to make fqname a named tuple, then there is no need access it's values by [] https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... MoinMoin/util/notifications.py:108: content = Content.create(contenttype) is it intended to be here?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... MoinMoin/util/notifications.py:108: content = Content.create(contenttype) On 2014/05/29 10:20:46, dimazest wrote: > is it intended to be here? I did it my mistake
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... MoinMoin/util/notifications.py:84: kw = dict(item_name=self.fqname[2], wiki_name=self.wiki_name, user_name=flaskg.user.name0) On 2014/05/29 10:20:46, dimazest wrote: > would it make sense to make fqname a named tuple, then there is no need access > it's values by [] You mean something like this: name should be accessed by fqname[NAME], field by fqname[FIELD] .. right?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... MoinMoin/util/notifications.py:84: kw = dict(item_name=self.fqname[2], wiki_name=self.wiki_name, user_name=flaskg.user.name0) more like self.fqname.name (have a look to https://docs.python.org/2/library/collections.html#collections.namedtuple ) however, this might be a too big change, and it's out of scope of this commit. On 2014/05/30 15:23:39, sksaurabhkathpalia wrote: > On 2014/05/29 10:20:46, dimazest wrote: > > would it make sense to make fqname a named tuple, then there is no need access > > it's values by [] > > You mean something like this: > name should be accessed by fqname[NAME], field by fqname[FIELD] .. > right?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... MoinMoin/util/notifications.py:84: kw = dict(item_name=self.fqname[2], wiki_name=self.wiki_name, user_name=flaskg.user.name0) On 2014/05/30 15:38:18, dimazest wrote: > more like self.fqname.name (have a look to > https://docs.python.org/2/library/collections.html#collections.namedtuple ) > > however, this might be a too big change, and it's out of scope of this commit. > > On 2014/05/30 15:23:39, sksaurabhkathpalia wrote: > > On 2014/05/29 10:20:46, dimazest wrote: > > > would it make sense to make fqname a named tuple, then there is no need > access > > > it's values by [] > > > > You mean something like this: > > name should be accessed by fqname[NAME], field by fqname[FIELD] .. > > right? > Then are you having some other idea or the current one is fine?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/util/notification... MoinMoin/util/notifications.py:84: kw = dict(item_name=self.fqname[2], wiki_name=self.wiki_name, user_name=flaskg.user.name0) yes. On 2014/05/30 16:26:36, sksaurabhkathpalia wrote: > On 2014/05/30 15:38:18, dimazest wrote: > > more like self.fqname.name (have a look to > > https://docs.python.org/2/library/collections.html#collections.namedtuple ) > > > > however, this might be a too big change, and it's out of scope of this commit. > > > > On 2014/05/30 15:23:39, sksaurabhkathpalia wrote: > > > On 2014/05/29 10:20:46, dimazest wrote: > > > > would it make sense to make fqname a named tuple, then there is no need > > access > > > > it's values by [] > > > > > > You mean something like this: > > > name should be accessed by fqname[NAME], field by fqname[FIELD] .. > > > right? > > > > Then are you having some other idea or the current one is fine?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/signalling/log.py File MoinMoin/signalling/log.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/signalling/log.py... MoinMoin/signalling/log.py:25: logging.info(u"item {0}:{1} modified".format(wiki_name, fqname[2])) On 2014/05/29 10:20:46, dimazest wrote: > wouldn't it be better to use logging like this: > > logging.info(u"item %s:%s modified", wiki_name, fqname[2]) > > then log aggregators, such as sentry, could group such messages. Then here also( https://bitbucket.org/thomaswaldmann/moin-2.0/src/600a9b4de684a67b5a247b3ff55... ) it needs to be changed. right?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/80001/MoinMoin/signalling/log.py File MoinMoin/signalling/log.py (right): https://codereview.appspot.com/93570043/diff/80001/MoinMoin/signalling/log.py... MoinMoin/signalling/log.py:25: logging.info(u"item {0}:{1} modified".format(wiki_name, fqname[2])) also, this is doesn't really relate to this change. we should keep an eye on this. On 2014/05/31 00:27:31, sksaurabhkathpalia wrote: > On 2014/05/29 10:20:46, dimazest wrote: > > wouldn't it be better to use logging like this: > > > > logging.info(u"item %s:%s modified", wiki_name, fqname[2]) > > > > then log aggregators, such as sentry, could group such messages. > > Then here also( > https://bitbucket.org/thomaswaldmann/moin-2.0/src/600a9b4de684a67b5a247b3ff55... > ) it needs to be changed. > right?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:188: value = fqname[2] is it possible to use more precise identifier names than "field" and "value"? https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:225: moin_name=app.cfg.interwikiname, item_name=fqname[2], user_name=u.name0) think how to avoid code duplication here, I see two almost identical statements.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:188: value = fqname[2] On 2014/05/31 11:28:19, dimazest wrote: > is it possible to use more precise identifier names than "field" and "value"? I was not able to get an any better name as sometimes field is NAME_EXACT and sometimes it is ITEMID. Also I used this as this is how fqname is defined fqname = CompositeName(namespace, field, value) So I think the current one should be fine because that goes with the semantic meaning of it
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:225: moin_name=app.cfg.interwikiname, item_name=fqname[2], user_name=u.name0) On 2014/05/31 11:28:19, dimazest wrote: > think how to avoid code duplication here, I see two almost identical statements. Can we do something like this if fqname[1] == NAME_EXACT: description = 'Name' elif fqname[1] == ITEMID: description = 'Itemid' subject = L_('[%(moin_name)s] Update of item with "%(description)s" "%(item_name)s" by %(user_name)s', moin_name=app.cfg.interwikiname, item_name=fqname[2], user_name=u.name0)
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:225: moin_name=app.cfg.interwikiname, item_name=fqname[2], user_name=u.name0) this makes sense. however, if fqname[1] is something else then there will be an exception. On 2014/05/31 11:38:43, sksaurabhkathpalia wrote: > On 2014/05/31 11:28:19, dimazest wrote: > > think how to avoid code duplication here, I see two almost identical > statements. > > Can we do something like this > if fqname[1] == NAME_EXACT: > description = 'Name' > elif fqname[1] == ITEMID: > description = 'Itemid' > subject = L_('[%(moin_name)s] Update of item with "%(description)s" > "%(item_name)s" by %(user_name)s', moin_name=app.cfg.interwikiname, > item_name=fqname[2], user_name=u.name0)
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:225: moin_name=app.cfg.interwikiname, item_name=fqname[2], user_name=u.name0) On 2014/05/31 11:40:45, dimazest wrote: > this makes sense. > > however, if fqname[1] is something else then there will be an exception. > > On 2014/05/31 11:38:43, sksaurabhkathpalia wrote: > > On 2014/05/31 11:28:19, dimazest wrote: > > > think how to avoid code duplication here, I see two almost identical > > statements. > > > > Can we do something like this > > if fqname[1] == NAME_EXACT: > > description = 'Name' > > elif fqname[1] == ITEMID: > > description = 'Itemid' > > subject = L_('[%(moin_name)s] Update of item with "%(description)s" > > "%(item_name)s" by %(user_name)s', moin_name=app.cfg.interwikiname, > > item_name=fqname[2], user_name=u.name0) > I think except NAME_EXACT, ITEMID only REVID can be there as mentioned in the https://bitbucket.org/thomaswaldmann/moin-2.0/src/600a9b4de684a67b5a247b3ff55... So I think one more elif statement can be added to accommodate that What do you think?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/100001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:225: moin_name=app.cfg.interwikiname, item_name=fqname[2], user_name=u.name0) then this code has to be updated accordingly when a new retrieval method is added. why not to use the fgname[1] directly. You can even uppercase it. Another option would be to use else clause. On 2014/05/31 11:46:29, sksaurabhkathpalia wrote: > On 2014/05/31 11:40:45, dimazest wrote: > > this makes sense. > > > > however, if fqname[1] is something else then there will be an exception. > > > > On 2014/05/31 11:38:43, sksaurabhkathpalia wrote: > > > On 2014/05/31 11:28:19, dimazest wrote: > > > > think how to avoid code duplication here, I see two almost identical > > > statements. > > > > > > Can we do something like this > > > if fqname[1] == NAME_EXACT: > > > description = 'Name' > > > elif fqname[1] == ITEMID: > > > description = 'Itemid' > > > subject = L_('[%(moin_name)s] Update of item with "%(description)s" > > > "%(item_name)s" by %(user_name)s', moin_name=app.cfg.interwikiname, > > > item_name=fqname[2], user_name=u.name0) > > > > I think except NAME_EXACT, ITEMID only REVID can be there as mentioned in the > https://bitbucket.org/thomaswaldmann/moin-2.0/src/600a9b4de684a67b5a247b3ff55... > So I think one more elif statement can be added to accommodate that > What do you think?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/120001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/120001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:226: moin_name=app.cfg.interwikiname, field = label.get(fqname[1], default = fqname[1]), item_name=fqname[2], user_name=u.name0) you should not put spaces around = when it's a function call (have a look to pep8) consider using something like: field=label.get(fqname[1], fqname[1])
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/120001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/120001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:226: moin_name=app.cfg.interwikiname, field = label.get(fqname[1], default = fqname[1]), item_name=fqname[2], user_name=u.name0) On 2014/05/31 13:02:30, dimazest wrote: > you should not put spaces around = when it's a function call (have a look to > pep8) > > consider using something like: > > field=label.get(fqname[1], fqname[1]) Yeah I forgot to check for pep8. Now I have corrected that
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/140001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/140001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:220: label = { rename to labels https://codereview.appspot.com/93570043/diff/140001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:226: moin_name=app.cfg.interwikiname, field=label.get(fqname[1], fqname[1]), item_name=fqname[2], user_name=u.name0) change field to label.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/140001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/140001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:220: label = { On 2014/05/31 13:49:01, dimazest wrote: > rename to labels Done. https://codereview.appspot.com/93570043/diff/140001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:226: moin_name=app.cfg.interwikiname, field=label.get(fqname[1], fqname[1]), item_name=fqname[2], user_name=u.name0) On 2014/05/31 13:49:01, dimazest wrote: > change field to label. Done.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/180001/MoinMoin/apps/frontend/vi... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/apps/frontend/vi... MoinMoin/apps/frontend/views.py:821: fqname = CompositeName(u'', NAME_EXACT, item_name) what's this u'' ? https://codereview.appspot.com/93570043/diff/180001/MoinMoin/signalling/log.py File MoinMoin/signalling/log.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/signalling/log.p... MoinMoin/signalling/log.py:23: def log_item_modified(app, fqname, **kwargs): so, you changed the log api here, but not in line 17 although it is pretty much equivalent? https://codereview.appspot.com/93570043/diff/180001/MoinMoin/signalling/log.p... MoinMoin/signalling/log.py:25: logging.info(u"item {0}:{1} modified".format(wiki_name, fqname[2])) see the CompositeName definition - it is a namedtuple so that we never have to use unreadable code like fqname[n]. :) please fix globally, at least in YOUR code. https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... File MoinMoin/util/_tests/test_notifications.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... MoinMoin/util/_tests/test_notifications.py:28: self.fqname = CompositeName(u'', NAME_EXACT, self.item_name) yet another magic u'' https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:189: value = fqname[2] _, field, value = fqname https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:226: subject = L_('[%(moin_name)s] Update of item with "%(label)s" "%(item_name)s" by %(user_name)s', so you translate the message here, but not the labels above?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... File MoinMoin/util/_tests/test_notifications.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... MoinMoin/util/_tests/test_notifications.py:28: self.fqname = CompositeName(u'', NAME_EXACT, self.item_name) On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > yet another magic u'' I just tried this with empty namespace. We can also have some other namespace such as u'ns' would that be fine?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/180001/MoinMoin/apps/frontend/vi... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/apps/frontend/vi... MoinMoin/apps/frontend/views.py:821: fqname = CompositeName(u'', NAME_EXACT, item_name) On 2014/05/31 16:31:53, Thomas.J.Waldmann wrote: > what's this u'' ? I think here I need to fetch the namespace and use that value. right?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:226: subject = L_('[%(moin_name)s] Update of item with "%(label)s" "%(item_name)s" by %(user_name)s', On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > so you translate the message here, but not the labels above? I am not able to get your point. Can you explain it again?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/180001/MoinMoin/apps/frontend/vi... File MoinMoin/apps/frontend/views.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/apps/frontend/vi... MoinMoin/apps/frontend/views.py:821: fqname = CompositeName(u'', NAME_EXACT, item_name) On 2014/05/31 16:42:40, sksaurabhkathpalia wrote: > On 2014/05/31 16:31:53, Thomas.J.Waldmann wrote: > > what's this u'' ? > > I think here I need to fetch the namespace and use that value. > right? Done. https://codereview.appspot.com/93570043/diff/180001/MoinMoin/signalling/log.py File MoinMoin/signalling/log.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/signalling/log.p... MoinMoin/signalling/log.py:23: def log_item_modified(app, fqname, **kwargs): On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > so, you changed the log api here, but not in line 17 although it is pretty much > equivalent? Done. https://codereview.appspot.com/93570043/diff/180001/MoinMoin/signalling/log.p... MoinMoin/signalling/log.py:25: logging.info(u"item {0}:{1} modified".format(wiki_name, fqname[2])) On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > see the CompositeName definition - it is a namedtuple so that we never have to > use unreadable code like fqname[n]. :) please fix globally, at least in YOUR > code. Done. https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... File MoinMoin/util/_tests/test_notifications.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... MoinMoin/util/_tests/test_notifications.py:28: self.fqname = CompositeName(u'', NAME_EXACT, self.item_name) On 2014/05/31 16:41:28, sksaurabhkathpalia wrote: > On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > > yet another magic u'' > > I just tried this with empty namespace. > We can also have some other namespace such as u'ns' > would that be fine? Used split_fqname to get fqname now https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:189: value = fqname[2] On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > _, field, value = fqname Done.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/200001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/200001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:189: terms = [Term(WIKINAME, app.cfg.interwikiname), Term(field, value), ] you can use fqname.field and fqname.value dirctly
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/200001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/200001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:189: terms = [Term(WIKINAME, app.cfg.interwikiname), Term(field, value), ] On 2014/06/01 08:39:39, dimazest wrote: > you can use fqname.field and fqname.value dirctly Done.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/170007/MoinMoin/signalling/log.py File MoinMoin/signalling/log.py (right): https://codereview.appspot.com/93570043/diff/170007/MoinMoin/signalling/log.p... MoinMoin/signalling/log.py:25: logging.info(u"item {0}:{1} modified".format(wiki_name, fqname.value)) if you only use the value of fqname, you lose information. https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:85: kw = dict(item_name=self.fqname.value, wiki_name=self.wiki_name, user_name=flaskg.user.name0, item_url=url_for_item(self.fqname)) fqname.value is not always an item_NAME (it can be also itemid or something else). if you only process .value, you are losing information. https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:225: moin_name=app.cfg.interwikiname, label=labels.get(fqname.field, fqname.field), item_name=fqname.value, user_name=u.name0) still no labels translation
Sign in to reply to this message.
On 2014/05/31 16:41:28, sksaurabhkathpalia wrote: > https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... > File MoinMoin/util/_tests/test_notifications.py (right): > > https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/_tests/test... > MoinMoin/util/_tests/test_notifications.py:28: self.fqname = CompositeName(u'', > NAME_EXACT, self.item_name) > On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > > yet another magic u'' > > I just tried this with empty namespace. > We can also have some other namespace such as u'ns' > would that be fine? If all your empty strings at such places mean something specific and the same, you maybe should use some named constant for that. u'' pretty much says nothing except "i am the empty string".
Sign in to reply to this message.
On 2014/05/31 16:46:30, sksaurabhkathpalia wrote: > https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... > File MoinMoin/util/notifications.py (right): > > https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... > MoinMoin/util/notifications.py:226: subject = L_('[%(moin_name)s] Update of item > with "%(label)s" "%(item_name)s" by %(user_name)s', > On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > > so you translate the message here, but not the labels above? > > I am not able to get your point. Can you explain it again? what part of "so you translate the message here, but not the labels above?" did you not understand?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/180001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:226: subject = L_('[%(moin_name)s] Update of item with "%(label)s" "%(item_name)s" by %(user_name)s', On 2014/05/31 16:31:54, Thomas.J.Waldmann wrote: > so you translate the message here, but not the labels above? Done.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:85: kw = dict(item_name=self.fqname.value, wiki_name=self.wiki_name, user_name=flaskg.user.name0, item_url=url_for_item(self.fqname)) On 2014/06/01 11:57:26, Thomas.J.Waldmann wrote: > fqname.value is not always an item_NAME (it can be also itemid or something > else). if you only process .value, you are losing information. But here it is only used to make message. It is not used for further processing.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:85: kw = dict(item_name=self.fqname.value, wiki_name=self.wiki_name, user_name=flaskg.user.name0, item_url=url_for_item(self.fqname)) But then messages might be not informative enough. On 2014/06/01 14:25:45, sksaurabhkathpalia wrote: > On 2014/06/01 11:57:26, Thomas.J.Waldmann wrote: > > fqname.value is not always an item_NAME (it can be also itemid or something > > else). if you only process .value, you are losing information. > > But here it is only used to make message. It is not used for further processing.
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/170007/MoinMoin/signalling/log.py File MoinMoin/signalling/log.py (right): https://codereview.appspot.com/93570043/diff/170007/MoinMoin/signalling/log.p... MoinMoin/signalling/log.py:25: logging.info(u"item {0}:{1} modified".format(wiki_name, fqname.value)) On 2014/06/01 11:57:26, Thomas.J.Waldmann wrote: > if you only use the value of fqname, you lose information. May be we can write like this logging.info(u"item with {0} {1}:{2} modified".format(fqname.field, wiki_name, fqname.value)) What do you say about this idea?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/170007/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:85: kw = dict(item_name=self.fqname.value, wiki_name=self.wiki_name, user_name=flaskg.user.name0, item_url=url_for_item(self.fqname)) On 2014/06/01 14:27:18, dimazest wrote: > But then messages might be not informative enough. > On 2014/06/01 14:25:45, sksaurabhkathpalia wrote: > > On 2014/06/01 11:57:26, Thomas.J.Waldmann wrote: > > > fqname.value is not always an item_NAME (it can be also itemid or something > > > else). if you only process .value, you are losing information. > > > > But here it is only used to make message. It is not used for further > processing. > Then here also we can use labels or just have the url of item only What do you suggest?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/260001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/260001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:219: subject = L_('[%(moin_name)s] Update of item with "%(label)s" "%(item_name)s" by %(user_name)s', use fqname instead of label as placeholder? why is item_name in here still? https://codereview.appspot.com/93570043/diff/260001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:220: moin_name=app.cfg.interwikiname, label=unicode(fqname), item_name=fqname.value, user_name=u.name0) you can not not whether fqname.value is the item's name. it depends on fqname.field. also, you already gave the full text representation of fqname, so why do you repeat a part of it?
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/260001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/260001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:219: subject = L_('[%(moin_name)s] Update of item with "%(label)s" "%(item_name)s" by %(user_name)s', On 2014/06/02 15:31:24, Thomas.J.Waldmann wrote: > use fqname instead of label as placeholder? > > why is item_name in here still? Oh sorry I forgot to remove that. Will do that
Sign in to reply to this message.
https://codereview.appspot.com/93570043/diff/260001/MoinMoin/util/notificatio... File MoinMoin/util/notifications.py (right): https://codereview.appspot.com/93570043/diff/260001/MoinMoin/util/notificatio... MoinMoin/util/notifications.py:219: subject = L_('[%(moin_name)s] Update of item with "%(label)s" "%(item_name)s" by %(user_name)s', On 2014/06/02 15:31:24, Thomas.J.Waldmann wrote: > use fqname instead of label as placeholder? > > why is item_name in here still? Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
