[issue11957] re.sub confusion between count and flags args

[Jakub Wilk]

Jakub Wilk added the comment:

+raise TypeError("sub() takes from 2 to 4 positional arguments "
+"but %d were given" % (4 + len(args)))

It's actually 3 to 5 for sub() and subn().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Jakub Wilk]

Changes by Jakub Wilk :


--
nosy: +jwilk

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Victor, I borrowed Guido's time machine and wrote patches implementing both 
your suggestions a half year ago.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[STINNER Victor]

STINNER Victor added the comment:

My issue #30072 has been marked as a duplicate of this one. Copy of my 
msg291650:

The re API seems commonly misused. Example passing a re flag to re.sub():

>>> re.sub("A", "B", "ahah", re.I)
'ahah'

No error, no warning, but it doesn't work. Oh, sub has 5 paramters, no 4...

I suggest to convert count and flags to keyword-only parameters. To not break 
the world, especially legit code passing the count parameter as a position 
argument, an option is to have a deprecation period if these two parameters are 
passed a positional-only parameter.

--

Another option would be to rely on the fact that re flags are now enums instead 
of raw integers, and so add basic type check...

Is there are risk of applications using re flags serialized by pickle from 
Pyhon < 3.6 and so getting integers?

Maybe the check should only be done if flags are passing as positional-only 
argument... but the implementation of such check seems may be overkill for such 
simple and performance-critical function, no?

See issue #30067 for a recent bug in the Python stdlib!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


Added file: http://bugs.python.org/file44826/re_deprecate_positional_count.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Here are two alternative patches. The first patch checks if count or maxsplit 
arguments are re.RegexFlag and raise TypeError if it is true. This makes 
misusing flags fail fast. The second patch deprecates passing count and 
maxsplit arguments as positional arguments. This imposes your to change your 
code (even if it is valid now) and makes hard misusing flags.

Unfortunately both ways slow down calling functions.

$ ./python -m perf timeit -s "import re" -- 're.split(":", ":a:b::c", 2)'

unpatched:   Median +- std dev: 2.73 us +- 0.09 us
check_flags_type:Median +- std dev: 3.74 us +- 0.09 us
deprecate_positional_count:  Median +- std dev: 10.6 us +- 0.2 us

$ ./python -m perf timeit -s "import re" -- 're.split(":", ":a:b::c", 
maxsplit=2)'

unpatched:   Median +- std dev: 2.78 us +- 0.07 us
check_flags_type:Median +- std dev: 3.75 us +- 0.10 us
deprecate_positional_count:  Median +- std dev: 2.86 us +- 0.08 us

--
stage:  -> patch review
versions: +Python 3.6, Python 3.7 -Python 3.5
Added file: http://bugs.python.org/file44825/re_check_flags_type.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Roundup Robot]

Roundup Robot added the comment:

New changeset 216e8b809e4e by Serhiy Storchaka in branch '3.5':
Issue #11957: Restored re tests for passing count and maxsplit as positional
https://hg.python.org/cpython/rev/216e8b809e4e

New changeset b39b09290718 by Serhiy Storchaka in branch '3.6':
Issue #11957: Restored re tests for passing count and maxsplit as positional
https://hg.python.org/cpython/rev/b39b09290718

New changeset da2c96cf2ce6 by Serhiy Storchaka in branch 'default':
Issue #11957: Restored re tests for passing count and maxsplit as positional
https://hg.python.org/cpython/rev/da2c96cf2ce6

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Ethan Furman]

Changes by Ethan Furman :


--
nosy: +ethan.furman

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Changes by Serhiy Storchaka storch...@gmail.com:


--
dependencies: +Add IntFlags

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Ezio Melotti]

Ezio Melotti added the comment:

I agree about 767fd62b59a9, there should be tests for args passed both by 
position and as keyword args.

Serhiy, do you think the enum solution is worth pursuing, or is it better to 
just turn those args to keyword-only (after a proper deprecation process)?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

I think that the enum solution is worth pursuing, and that we need general 
class which represents the set of specified named flags. I'm working on 
implementation of enum.IntFlags.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Roundup Robot]

Roundup Robot added the comment:

New changeset 767fd62b59a9 by Victor Stinner in branch 'default':
Issue #11957: Explicit parameter name when calling re.split() and re.sub()
https://hg.python.org/cpython/rev/767fd62b59a9

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[STINNER Victor]

STINNER Victor added the comment:

I suggest to make the 2 last parameters of re.sub(), re.subn() and re.split() 
parameters as keyword-only. It will break applications using count and maxsplit 
parameters as index parameters, but it's easy to fix these applications if they 
want to support also Python 3.5.

I checked Python 2.6: the name of the maxsplit and count parameters didn't 
change. So it's possible to write code working on Python 2.6-3.5 if the 
parameter name is explicitly used:

* re.sub(a, a, a, count=1)
* re.subn(a, a, a, count=1)
* re.split(a, a, maxsplit=1)

The flags parameter was added to re.sub(), re.subn() and re.split() functions 
in Python 2.7:

* https://docs.python.org/2.7/library/re.html#re.sub
* https://docs.python.org/2.7/library/re.html#re.subn
* https://docs.python.org/2.7/library/re.html#re.split

See my attached re_keyword_only.patch:

* sub(), subn(): count and flags become keyword-only parameters
* split(): maxsplit and flags become keyword-only parameters

--
keywords: +patch
nosy: +haypo
Added file: http://bugs.python.org/file37067/re_keyword_only.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[STINNER Victor]

STINNER Victor added the comment:

Confusion between count/maxplit and count parameters is common, duplicated 
issues:

* Issue #22760
* Issue #17663
* Issue #15537
* Issue #12875
* Issue #12078
* Issue #11947

See also issue #13385 which proposed an explicit re.NOFLAGS flag.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[STINNER Victor]

Changes by STINNER Victor victor.stin...@gmail.com:


--
nosy: +serhiy.storchaka

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Thank you for your patch Valentina. But it makes flags combinations not 
pickleable.

 import re, pickle
 pickle.dumps(re.I|re.S, 3)
Traceback (most recent call last):
  File stdin, line 1, in module
_pickle.PicklingError: Can't pickle enum 'SubFlag': attribute lookup SubFlag 
on sre_constants failed
 pickle.dumps(re.I|re.S, 4)
Traceback (most recent call last):
  File stdin, line 1, in module
_pickle.PicklingError: Can't pickle enum 'SubFlag': attribute lookup 
BaseFlags.__or__.locals.SubFlag on sre_constants failed

And I'm afraid that creating new class in the | operator can affect 
performance.

--
versions: +Python 3.5 -Python 2.7, Python 3.3, Python 3.4

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

As for 767fd62b59a9, I doubt that changing positional arguments to keyword 
argumennts in tests is justified. This can hide a bug.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

And again about patch_11957. I afraid that testing isinstance(count, 
sre_constants.BaseFlags) on every re.sub() call will hit performance too.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Valentina Mukhamedzhanova]

Valentina Mukhamedzhanova added the comment:

Please see my patch, I have changed flags to be instances of IntEnum and added 
a check to re.sub, re.subn and re.split. The patch contains some tests. This 
solution also allowed me to discover several bugs in the standard library, and 
I am going to create tickets for them shortly.

--
nosy: +umi
Added file: http://bugs.python.org/file30801/patch_11957

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Valentina Mukhamedzhanova]

Changes by Valentina Mukhamedzhanova umi...@gmail.com:


Removed file: http://bugs.python.org/file30801/patch_11957

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Valentina Mukhamedzhanova]

Changes by Valentina Mukhamedzhanova umi...@gmail.com:


Added file: http://bugs.python.org/file30813/patch_11957

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Valentina Mukhamedzhanova]

Changes by Valentina Mukhamedzhanova umi...@gmail.com:


Removed file: http://bugs.python.org/file30813/patch_11957

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Valentina Mukhamedzhanova]

Changes by Valentina Mukhamedzhanova umi...@gmail.com:


Added file: http://bugs.python.org/file30821/patch_11957

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[STINNER Victor]

Changes by STINNER Victor victor.stin...@gmail.com:


--
components:  -Unicode

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Raymond Hettinger]

Changes by Raymond Hettinger raymond.hettin...@gmail.com:


--
assignee: rhettinger - ezio.melotti

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Mike Milkin]

Mike Milkin added the comment:

I like option #2, and I was thinking of working on it today, poke me if anyone 
has a problem with this.

--
nosy: +mmilkin

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Mike Milkin]

Mike Milkin added the comment:

There is no sane way to issue a warning without changing the signature and we 
don't want to change the signature without issuing a deprecation warning for 
the function, so sadly option 3 is the only way for this to work, (Im going to 
not touch this till ENUMS are merged in.)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Ezio Melotti]

Ezio Melotti added the comment:

Can't you use *args and **kwargs and then raise a deprecation warning if count 
and/or flags are in args?
Even if enums are merged in, there might still be issues depending on their 
implementation.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Mike Milkin]

Mike Milkin added the comment:

We could do that but we would be changing the signature before adding the 
warning

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Ezio Melotti]

Ezio Melotti added the comment:

The change would still be backwards compatible (even though inspect.signature 
and similar functions might return something different).  Note that I'm not 
saying that's the best option, but it should be doable.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Ezio Melotti]

Changes by Ezio Melotti ezio.melo...@gmail.com:


--
type:  - enhancement
versions: +Python 3.4 -Python 3.1, Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Eric Snow]

Changes by Eric Snow ericsnowcurren...@gmail.com:


--
nosy:  -eric.snow

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Eric Snow]

Changes by Eric Snow ericsnowcurren...@gmail.com:


--
nosy: +eric.snow

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Ezio Melotti]

Ezio Melotti ezio.melo...@gmail.com added the comment:

See also #12888 for an error in the stdlib caused by this.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

I’d favor 1) or 2) over 3).  Ints are short and very commonly used for flags.

--
nosy: +eric.araujo

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Raymond Hettinger]

Changes by Raymond Hettinger raymond.hettin...@gmail.com:


--
assignee:  - rhettinger
nosy: +rhettinger

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Terry J. Reedy]

Terry J. Reedy tjre...@udel.edu added the comment:

I like the idea of an internal REflag class with __new__, __or__, and 
__repr__==__str__. Str(re.A|re.L) might print as
REflag: re.ASCII | re.IGNORE
If it is *not* an int subclass, any attempt to use or mix with an int would 
raise. I checked and the doc only promises that flags can be or'ed. An __and__ 
method might be added if it were thought that people currently use  to check 
for flags set, though that is not currently promised.

--
nosy: +terry.reedy

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Matthew Barnett]

Matthew Barnett pyt...@mrabarnett.plus.com added the comment:

Something like re.Flag ASCII | IGNORE may be more Pythonic.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Terry J. Reedy]

Terry J. Reedy tjre...@udel.edu added the comment:

Agreed, if we go that route.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue11957] re.sub confusion between count and flags args

[Ezio Melotti]

Ezio Melotti ezio.melo...@gmail.com added the comment:

Since this has been reported already several times (see e.g. #11947), and it's 
a fairly common mistake, I think we should do something to avoid it.

A few possibilities are:
  1) add a warning in the doc;
  2) make count and flag keyword-only argument (raising a deprecation warning 
in 3.3 and actually change it later);
  3) change the regex flags to some object that can be distinguished from ints 
and raise an error when a flag is passed to count;

--
nosy: +ezio.melotti, mrabarnett
title: re.sub problem with unicode string - re.sub confusion between count and 
flags args
versions: +Python 3.1, Python 3.2, Python 3.3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue11957
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com