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

#20173: Derby #4: Convert 53 sites to Argument Clinic across 5 files

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 1 month ago by larry
Modified:
4 years, 10 months ago
Reviewers:
storchaka
CC:
loewis, larry, christian.heimes, devnull_psf.upfronthosting.co.za, Zach Ware, storchaka, vajrasky
Visibility:
Public.

Patch Set 1 #

Total comments: 2

Patch Set 2 #

Total comments: 3

Patch Set 3 #

Total comments: 3

Patch Set 4 #

Total comments: 7

Patch Set 5 #

Patch Set 6 #

Total comments: 4

Patch Set 7 #

Patch Set 8 #

Total comments: 18

Patch Set 9 #

Patch Set 10 #

Patch Set 11 #

Patch Set 12 #

Patch Set 13 #

Patch Set 14 #

Patch Set 15 #

Patch Set 16 #

Patch Set 17 #

Patch Set 18 #

Patch Set 19 #

Patch Set 20 #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Modules/clinic/_codecsmodule.c.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +1413 lines, -1 line 0 comments Download
Modules/_codecsmodule.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 25 chunks +629 lines, -615 lines 10 comments Download

Messages

Total messages: 12
larry
Thanks for contributing! Just a minor thing. http://bugs.python.org/review/20173/diff/10492/Modules/sha1module.c File Modules/sha1module.c (right): http://bugs.python.org/review/20173/diff/10492/Modules/sha1module.c#newcode506 Modules/sha1module.c:506: [ You ...
6 years, 1 month ago #1
larry
http://bugs.python.org/review/20173/diff/10492/Modules/sha1module.c File Modules/sha1module.c (right): http://bugs.python.org/review/20173/diff/10492/Modules/sha1module.c#newcode506 Modules/sha1module.c:506: [ On 2014/01/08 12:40:11, larry wrote: > You shouldn't ...
6 years, 1 month ago #2
larry
6 years, 1 month ago #3
larry
http://bugs.python.org/review/20173/diff/10502/Modules/sha1module.c File Modules/sha1module.c (right): http://bugs.python.org/review/20173/diff/10502/Modules/sha1module.c#newcode391 Modules/sha1module.c:391: args: object "args" is a confusing name for this ...
6 years, 1 month ago #4
larry
Some more touch-up comments. Getting close! http://bugs.python.org/review/20173/diff/10512/Modules/sha1module.c File Modules/sha1module.c (right): http://bugs.python.org/review/20173/diff/10512/Modules/sha1module.c#newcode388 Modules/sha1module.c:388: _sha1.SHA1.update The name ...
6 years, 1 month ago #5
larry
Needs some work. http://bugs.python.org/review/20173/diff/10513/Modules/sha256module.c File Modules/sha256module.c (right): http://bugs.python.org/review/20173/diff/10513/Modules/sha256module.c#newcode41 Modules/sha256module.c:41: class _sha256.SHA256 The name of the ...
6 years, 1 month ago #6
larry
Same feedback as for your other conversions. At least you're consistent! http://bugs.python.org/review/20173/diff/10515/Modules/md5module.c File Modules/md5module.c (right): ...
6 years, 1 month ago #7
larry
Thanks for plowing through this! That was a lot of conversion. In general I can ...
6 years, 1 month ago #8
larry
Looks great! Just a few suggestions. Maybe hold off until #23967 gets accepted (or not)? ...
4 years, 10 months ago #9
storchaka_gmail.com
Thank you for your review Larry! http://bugs.python.org/review/20173/diff/14624/Modules/_codecsmodule.c File Modules/_codecsmodule.c (right): http://bugs.python.org/review/20173/diff/14624/Modules/_codecsmodule.c#newcode121 Modules/_codecsmodule.c:121: encoding: str(c_default="NULL") = ...
4 years, 10 months ago #10
larry
http://bugs.python.org/review/20173/diff/14624/Modules/_codecsmodule.c File Modules/_codecsmodule.c (right): http://bugs.python.org/review/20173/diff/14624/Modules/_codecsmodule.c#newcode121 Modules/_codecsmodule.c:121: encoding: str(c_default="NULL") = "utf-8" On 2015/04/17 19:34:04, storchaka wrote: ...
4 years, 10 months ago #11
storchaka_gmail.com
4 years, 10 months ago #12
http://bugs.python.org/review/20173/diff/14624/Modules/_codecsmodule.c
File Modules/_codecsmodule.c (right):

http://bugs.python.org/review/20173/diff/14624/Modules/_codecsmodule.c#newcod...
Modules/_codecsmodule.c:121: encoding: str(c_default="NULL") = "utf-8"
On 2015/04/22 07:17:32, larry wrote:
> On 2015/04/17 19:34:04, storchaka wrote:
> > On 2015/04/16 23:37:04, larry wrote:
> > > If #23967 is accepted, the Python default should be
> > >   sys.getdefaultencoding()
> > 
> > It is always "utf-8" in Python 3. sys.getdefaultencoding() and
> > PyUnicode_GetDefaultEncoding() is a legacy of Python 2.
> 
> sys.getdefaultencoding() is not deprecated in the manual, and it still calls
> PyUnicode_GetDefaultEncoding().  Until someone rips out all these calls to it
> and replaces them with the hard-coded string "utf-8", I think we should stick
> with this being dynamic.

PyUnicode_GetDefaultEncoding() just returns "utf-8", and there is no longer
sys.setdefaultencoding(). For now sys.setdefaultencoding() is used only in the
calendar module.

See also a docstring of str.encode().

http://bugs.python.org/review/20173/diff/14624/Modules/_codecsmodule.c#newcod...
Modules/_codecsmodule.c:407: final: int(c_default="0") = False
On 2015/04/22 07:17:32, larry wrote:
> On 2015/04/17 19:34:04, storchaka wrote:
> > Semantically this is bool, but old code (here and in many other places) use
> > format unit "i". May be add "intbool" or "boolint" converter and then slowly
> > replace its usage with "bool"?
> 
> The only difference between "bool" (format unit 'p') and "int" (format unit
'i')
> is that bool accepts more types.  It will accept strings, or floats, or lists,
> or dicts... anything that supports truth testing.
> 
> If you're worried that accepting more types for these parameters will cause
> problems, okay, leave it as 'int'.  But I don't understand why a "boolint"
> converter would help.

The downside of bool converter is that it is harder to extend it. We can't add a
support of say a tuple, or callable, or string, or ternary logic value without
potential breaking some code.

The boolint converter would allow to not specify the c_default parameter for
every optional boolint parameter.
Sign in to reply to this message.

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