Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(132)

#14423: Getting the starting date of iso week from a week number and a year.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years ago by esbenab
Modified:
7 years, 11 months ago
Reviewers:
merwok
CC:
lemburg, mark.dickinson, sasha, AntoinePitrou, Esben.Agerbæk.Black, erik_cederstrand.dk
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Patch Set 3 #

Patch Set 4 #

Total comments: 4

Patch Set 5 #

Patch Set 6 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/datetime.rst View 1 chunk +9 lines, -0 lines 0 comments Download
Lib/datetime.py View 1 2 3 4 5 2 chunks +16 lines, -0 lines 0 comments Download
Lib/test/datetimetester.py View 3 4 5 3 chunks +44 lines, -0 lines 0 comments Download
Modules/_datetimemodule.c View 1 chunk +44 lines, -0 lines 0 comments Download

Messages

Total messages: 1
eric.araujo
7 years, 12 months ago #1
Thanks for the patch.  Some comments follow.

http://bugs.python.org/review/14423/diff/4562/16180
File Lib/datetime.py (right):

http://bugs.python.org/review/14423/diff/4562/16180#newcode901
Lib/datetime.py:901: """Construct a self from ISO year, week number and weekday
(default 1)
s/self/date/

You can remove the default from the docstring, introspection and doc tools
already know how to get the function’s signature.

http://bugs.python.org/review/14423/diff/4562/16180#newcode904
Lib/datetime.py:904: raise ValueError("'day' invalid, acceptable range 1-7")
Maybe you can print the day in the error message, which helps debugging.

http://bugs.python.org/review/14423/diff/4562/16180#newcode906
Lib/datetime.py:906: raise ValueError("'week' invalid, accaptable range 1-53")
acceptable*

http://bugs.python.org/review/14423/diff/4562/16180#newcode913
Lib/datetime.py:913: raise OverflowError("'week' within range but overflows to
next year")
OverflowError seems inappropriate here, why not using ValueError?
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+