Roundup Tracker - Issues

Issue 2550871

classification
Title: Extending translation ability (for date at least)
Type: behavior Severity: normal
Components: Versions: devel
process
Status: new Resolution:
Dependencies: Introduce an Integer class, Small fix to date format() function
View: 2550886

View: 2550887
Superseder:
Assigned To: Nosy List: antmail, jerrykan, rouilj
Priority: normal Keywords: patch

Created on 2015-02-26 16:38 by antmail, last changed 2016-06-05 10:46 by antmail.

Files
File name Uploaded Description Edit Remove
ext_translation.diff antmail, 2015-02-26 16:38 Fix many translation issues and localize date format input.
i18n_wow.diff antmail, 2016-06-03 17:40
Messages
msg5228 Author: [hidden] (antmail) Date: 2015-02-26 16:38
Roundup tracker doesn't support date input in localized notation.
You can't input date such as DD.MM.YYYY.

Patches do the following:

i18n.py
- Add class I18NStaff. 
  Introduce I18NStaff.date_input_re variable which hold locale specific
  regular expression for date parsing.
  Introduce I18NStaff.date_input_desc variable which hold locale specific
  desciption of date/time format.
  Pattern and description can be localized through language translation
file/
  (rebuild roundup's language file: cd locale && gmake template after
patch).
  In apropriate language file find:
"'''^
((?P<y>\d\d\d\d)([/-](?P<m>\d\d?)([/-](?P<d>\d\d?))?)? # yyyy[-mm[-dd]]
|(?P<a>\d\d?)[/-](?P<b>\d\d?))?              # or mm-dd
(?P<n>\.)?                                   # .
(((?P<H>\d?\d):(?P<M>\d\d))?(:(?P<S>\d\d?(\.\d+)?))?)?  # hh:mm:ss
(?P<o>[\d\smywd\-+]+)?                       # offset
$'''"
  and replace it to desired expression which will parse your country
specific date/time format.
  Regular expression must contain y,m,d group for full date, a,b - for
m-d variant, H,M,S for time
  Also replace 'Proper date/time format is "yyyy-mm-dd",
"yyyy-mm-dd.HH:MM:SS.SSS"' to
  something which describe date/time format.
- combine I18NStaff with RoundupTranslation, RoundupNullTranslation;

date.py
 - remove date regular expression variable (date_re) (it in i18n.py now)
 - set date __init__ default parameter from translator=i18n to
translator=i18n.translation, 
   which is more accurate.
 - remove date_re parameter from Date.set()
 - do use localized translator.date_input_re and
translator.date_input_desc in Date.set()

cgi/templating.py
 - in DateHtmlProperty when _value is a string not converted to date,
plain() and now() throw
   exception because of calling _value.local(). Recently somebody fix it
in plain() 
   ( except AttributeError: ...). Repeat this fix.

cgi/client.py
 Somebody already do the same thing as i want:
 - hide _error_message.
 - define function add_error_message.
 So, no need to change.
 By the way, there is a some issue. Jinja2 template bundled with
roundup1.5 think that error_msg is a string.
But it is a list. So, _error_msg in page has a bad format. When errors
are localized they become unreadable.

cgi/form_parser.py
 This is a transit point on the way to hyperdb. The goal is to pass 
 translator object to rawToHyperDB method. So
 - introduce 'translator' variable, which is initialized from 'client'
object;
 - define wrapper function rawToHyperDB which respect translator object;
 - replace direct call hyperdb.rawToHyperDB to wrapper.

hyperdb.py
 - import translation from i18n for default translator value;
 - add parameter 'translator' to rawToHyperdb function and pass it to
from_raw() method;
 - pass translator to Date object.
msg5249 Author: [hidden] (jerrykan) Date: 2015-03-03 13:16
For reference it looks like some some initials discussion around this
(or a very similar) patch can be found at issue2550825.

I'm not very familiar with the i18n stuff, but I'll try to have a look
at this patch in this patch in the next week or so to at least rovide
some feedback. Feel free to bump the issue if I forget.
msg5333 Author: [hidden] (rouilj) Date: 2015-06-28 04:05
John/jerrykahn bump. Have you had a chance to look at this?

issue2550888

may need some feedback on this.
msg5346 Author: [hidden] (jerrykan) Date: 2015-07-23 04:06
Hello John,

I started to have a bit of a look at this patch but ended up spending 
the time trying to figure out how to make roundup display in another 
language (as a native English speaker I've not had to deal with i18n 
stuff much). By the time I got the i18n stuff working I ran out of time 
and haven't had a chance to get back to looking at it.

One thing that may help in reviewing that patch is if Anthony could 
outline some steps to show what this patch does. ie. a set of steps to 
show what the current behaviour is, and another set of steps showing the 
difference in behaviour after the patch is applied. Then I may have a 
bit more of an idea about what needs to be checked.

The changes hit a lot of different files, which raises concerns about if 
this is the best approach (I'm not saying it is a wrong approach, just 
that it needs looking at with an eye to future maintainability).

At a quick glance there are also a number of unrelated changes (ie. 
random white-space fixes) in the patch which also raises flags that the 
patch may include unrelated changes and the patch will need to be 
cleaned up before I could be applied[1].

SeeYa,
John.

[1] I am of the opinion that white-space changes should only be made 
when code bordering that white-space is being modified. Otherwise it has 
a tendency to introduce bugs that are totally unrelated to intended 
changes being made and could have been avoided.

On 28/06/15 14:05, John Rouillard wrote:
>
> John Rouillard added the comment:
>
> John/jerrykahn bump. Have you had a chance to look at this?
>
> issue2550888
>
> may need some feedback on this.
>
> ----------
> nosy: +rouilj
>
> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550871>
> ________________________________________________
>
msg5552 Author: [hidden] (rouilj) Date: 2016-04-30 03:16
Anthony,

Can you address the concerns Jerry brought up and

redo the patch removing unneeded whitespace changes
and merge the patch from issue2550887

as well as provide some steps we can use to compare before and after
behavior of the patch.

-- rouilj
msg5573 Author: [hidden] (antmail) Date: 2016-06-03 17:25
Yes,  this  patches  hit  many  files  but  mostly to fix translation
handling.

There  are  single  behavior change: user can set date field value in native
format such as dd.mm.yyyy.
From  system  administrator/developer/translator  point  of  view also
there  is a single change:  regular expression to parse date value may
be localized in language-translation file.

To get it you need:
1) apply patch;

2) regenerate roundup.pot file
In 'locale' dir of roundup source:
find ../roundup/ -path "../roundup/scripts/" -prune -o -iname "*.py" > rs.files
xgettext -w 80 -o roundup.pot -f rs.files
rm rs.files

3) merge new roundup.pot with target language .po file
msgmerge -U --suffix=.bak target_language.po roundup.pot

4)    In  target  language  .po  file  find  the  msgid  and  add  the
translation:
#: ../roundup/i18n.py:90
msgid ""
"^\n"
"            ((?P<y>\\d\\d\\d\\d)([/-](?P<m>\\d\\d?)([/-](?P<d>\\d\\d?))?)? # "
"yyyy[-mm[-dd]]\n"
"            |(?P<a>\\d\\d?)[/-](?P<b>\\d\\d?))?              # or mm-dd\n"
"            (?P<n>\\.)?                                   # .\n"
"            (((?P<H>\\d?\\d):(?P<M>\\d\\d))?(:(?P<S>\\d\\d?(\\.\\d+)?))?)?  "
"# hh:mm:ss\n"
"            (?P<o>[\\d\\smywd\\-+]+)?                       # offset\n"
"        $"
msgstr ""
"^\\s*\n"
"(((?P<d>\\d\\d?)\\.)?((?P<m>\\d\\d?)\\.)(?P<y>\\d\\d\\d\\d) # [dd.]"
"[mm.]yyyy \n"
"|(?P<b>\\d\\d)\\.(?P<a>\\d\\d))?  #or d.m\n"
"(?P<n>\\.)? # may be . (now)\n"
"(\\s+((?P<H>\\d\\d?):(?P<M>\\d\\d))?(:(?P<S>\\d\\d?(\\.\\d+)?))?)? #hh:mm:"
"ss\n"
"(?P<o>[\\d\\smywd\\-+]+)?                       # offset\n"
"\\s*$"

!!! You can skip 2),3) and just add to target language .po file lines from 4) !!!

5) Compile target  language .po file:
msgfmt --statistics -o target_language.mo target_language.po

6) Place target_language.mo file in roundup tracker subdir 'locale'

7)  In config.ini of roundup tracker set 'language' to target_language
such as:
language = target_language.UTF8

8) restart roundup-tracker

Now you can input date as dd.mm.yyyy, mm.yyyy etc.

> John Rouillard added the comment:

> Anthony,

> Can you address the concerns Jerry brought up and

> redo the patch removing unneeded whitespace changes
> and merge the patch from issue2550887

> as well as provide some steps we can use to compare before and after
> behavior of the patch.

> -- rouilj

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550871>
> ________________________________________________
msg5574 Author: [hidden] (antmail) Date: 2016-06-03 17:40
The patch with no whitespace problems (i hope) attached here.

Also,  the code base of date.py has changed after this patch
was   written   so   i   did  some  merging.  Unfortunately  I don't know what to get
from Mercurial VCS to help other people to apply this patch.

> John Rouillard added the comment:

> Anthony,

> Can you address the concerns Jerry brought up and

> redo the patch removing unneeded whitespace changes
> and merge the patch from issue2550887

> as well as provide some steps we can use to compare before and after
> behavior of the patch.

> -- rouilj

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550871>
> ________________________________________________
msg5578 Author: [hidden] (rouilj) Date: 2016-06-05 05:28
Hi Anthony:

Anthony said:
> The patch with no whitespace problems (i hope) attached here.

It looks cleaner than the original. Did you merge the patch from
issue2550887 into it? If so I will close issue2550887.

> Also, the code base of date.py has changed after this patch
> was written so i did  some  merging.  Unfortunately  I don't
> know what to get from Mercurial VCS to help other people to
> apply this patch.

I don't understand what you mean here. Are you saying this patch
will not apply to the current head of the default branch?
John K. any ideas?

I do have a couple of questions about the patch itself that I don't
understand.

On part of the patch has this:

         if allowdate and info['D'] is not None:
-            now = Date('.')
+            now = Date()

why was this change needed?

Also I see:

-            raise HyperdbValueError, _('property %s: %r is not a number')%(
+            raise HyperdbValueError, translator.gettext('property %s:
%r is not a number')%(
                 kw['propname'], value)

in a number of places. What does it do? I thought _('....') is
replaced by the localized version of .... and the items in the tuple
are just substituted into the localized string. IIUC that is exactly
the same thing that

   translator.gettext('...')%(tuple)

will do. Translator.gettext won't translate any values in the tuple.
So what does this change accomplish?

Also will I need another patch for the Integer patch I just
committed (reversing what I took out in issue2550886).
The patch will add the translator:

-    def from_raw(self, value, **kw):
+    def from_raw(self, value, translator=translation, **kw):

to the Integer::from_raw function and insert translator.gettext to
replace _ .

-- rouilj
msg5580 Author: [hidden] (antmail) Date: 2016-06-05 10:46
> Hi Anthony:

> Anthony said:
>> The patch with no whitespace problems (i hope) attached here.

> It looks cleaner than the original. Did you merge the patch from
> issue2550887 into it? If so I will close issue2550887.

Unfortunately, i don't know how to combine patch.  May be the best way
is  to  attach patch from issue2550887 to  issue2550871 and then close
issue2550887?

>> Also, the code base of date.py has changed after this patch
>> was written so i did  some  merging.  Unfortunately  I don't
>> know what to get from Mercurial VCS to help other people to
>> apply this patch.

> I don't understand what you mean here. Are you saying this patch
> will not apply to the current head of the default branch?
Yes,  when  I  pulled  changes  there  is  a need of manual merging in
date.py.

> I do have a couple of questions about the patch itself that I don't
> understand.

> On part of the patch has this:

>          if allowdate and info['D'] is not None:
> -            now = Date('.')
> +            now = Date()

> why was this change needed?

Sorry,  I don't remember.  I can only say that this is not crucial
for   patch  functionality.   Probably, i was touching something there
but  then  not reverted it back fully.  The choice of Date with period
argument  and  Date  alone  depends  of   what  seems more cleanly, of
course.

> Also I see:

> -            raise HyperdbValueError, _('property %s: %r is not a number')%(
> +            raise HyperdbValueError, translator.gettext('property %s:
> %r is not a number')%(
>                  kw['propname'], value)

> in a number of places. What does it do? I thought _('....') is
> replaced by the localized version of .... and the items in the tuple
> are just substituted into the localized string. IIUC that is exactly
> the same thing that

>    translator.gettext('...')%(tuple)

> will do. Translator.gettext won't translate any values in the tuple.
> So what does this change accomplish?

A  I  remember this is the main part of the patch. The main problem is
that  localized  translator  object from context of request is lost as
they   move   in   depth   of  roundup  code.  All translation in base
models  (like date.py) are tied to _ variable which is defaulted to no
translation.
So the main difference of
 -            raise HyperdbValueError, _('property %s: %r is not a number')%(
 +            raise HyperdbValueError, translator.gettext('property %s:
is to use translator.gettext() istead of _().

The  patch  is to carefully lower the translator object to  the
depth of code and then use it.

> Also will I need another patch for the Integer patch I just
> committed (reversing what I took out in issue2550886).
> The patch will add the translator:

> -    def from_raw(self, value, **kw):
> +    def from_raw(self, value, translator=translation, **kw):

> to the Integer::from_raw function and insert translator.gettext to
> replace _ .

There  is  no problem. Just the  running tests will not output translated
errors.

> ________________________________________________
> Roundup tracker <issues@roundup-tracker.org>
> <http://issues.roundup-tracker.org/issue2550871>
> ________________________________________________
History
Date User Action Args
2016-06-05 10:46:06antmailsetmessages: + msg5580
2016-06-05 05:29:27rouiljsetpriority: normal
dependencies: + Introduce an Integer class, Small fix to date format() function
2016-06-05 05:28:03rouiljsetmessages: + msg5578
2016-06-03 17:40:31antmailsetfiles: + i18n_wow.diff
messages: + msg5574
2016-06-03 17:25:04antmailsetmessages: + msg5573
2016-04-30 03:16:15rouiljsetmessages: + msg5552
2015-07-23 04:06:02jerrykansetmessages: + msg5346
2015-07-01 19:03:07jerrykanlinkissue2550888 dependencies
2015-07-01 19:01:35jerrykanlinkissue2550887 dependencies
2015-06-28 04:05:38rouiljsetnosy: + rouilj
messages: + msg5333
2015-03-03 13:16:42jerrykansetmessages: + msg5249
2015-03-03 13:08:25jerrykansetnosy: + jerrykan
2015-02-26 16:38:18antmailcreate