Merge lp:~openerp-dev/openobject-server/trunk-dates_py-niv into lp:openobject-server
Proposed by
Nicolas Vanhoren (OpenERP)
Status: | Needs review |
---|---|
Proposed branch: | lp:~openerp-dev/openobject-server/trunk-dates_py-niv |
Merge into: | lp:openobject-server |
Diff against target: |
47 lines (+43/-0) 1 file modified
openerp/tools/dates.py (+43/-0) |
To merge this branch: | bzr merge lp:~openerp-dev/openobject-server/trunk-dates_py-niv |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Nicolas Vanhoren (OpenERP) (community) | Needs Resubmitting | ||
Vo Minh Thu (community) | Needs Fixing | ||
Review via email: mp+162738@code.launchpad.net |
Description of the change
Added dates.py.
To post a comment you must log in.
Unmerged revisions
- 4873. By Nicolas Vanhoren (OpenERP)
-
typo
- 4872. By Nicolas Vanhoren (OpenERP)
-
changed the way we handle microseconds + removed xxx_to_str
- 4871. By Nicolas Vanhoren (OpenERP)
-
Fix problem with milliseconds coming from postgresql
- 4870. By Nicolas Vanhoren (OpenERP)
-
[IMP] added dates.py
- 4869. By Nicolas Vanhoren (OpenERP)
-
merge trunk
Hi,
- Why all the `if not str: return str` and `if not obj: return False` ? What can they be when being falsy ? None, False, empty string ?
Actually I had a look at the code in the server and in the addons, and I'm not sure the case where we provide falsy values are common enough. At least, if I see `some_date. strptime( )`, I know for sure that `some_date` can't be falsy (which is good).
- Same problem with the `.split('.')`. Again it seems a bit lousy.
- A comment such as "The datetime instance should not have an attached timezone and be in UTC." would be better complemented with an assertion.
- All the comments about "OpenERP 6.1 specification" or "Times in OpenERP are naive times." would be better in a single module-level docstring.
Overall I'm not sure we gain much with this helper functions:
- `time_to_str(obj)` is not much shorter than `obj.strftime( TIME_FORMAT) ` and is more sloppy.
- With a quick grep, it seems str_to_date() would be of very little use...
I'm not against such helpers but I'm aginst having the core library used as a convenience library that doesn't even use its own helpers, or have an additional way to do things inconsistently:
We would have the DEFAULT_ SERVER_ *_FORMAT, *_FORMAT, the '%Y-%m-%d' literals, and your helpers.
Also, even by trying to replace occurences of DEFAULT_ SERVER_ *_FORMAT in the code, we could not remove them all (for instance we use them also with time instead of datetime).
Maybe we should try in theserver see if using your helpers really help.