Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify tests for str.format and string.Formatter #58227

Open
ncoghlan opened this issue Feb 15, 2012 · 20 comments
Open

Unify tests for str.format and string.Formatter #58227

ncoghlan opened this issue Feb 15, 2012 · 20 comments
Labels
3.10 only security fixes tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 14019
Nosy @terryjreedy, @ncoghlan, @vstinner, @ericvsmith, @ezio-melotti, @merwok, @nanjekyejoannah

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2012-02-15.08:15:04.384>
labels = ['type-feature', 'tests', '3.10']
title = 'Unify tests for str.format and string.Formatter'
updated_at = <Date 2020-11-10.13:53:04.283>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2020-11-10.13:53:04.283>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Tests']
creation = <Date 2012-02-15.08:15:04.384>
creator = 'ncoghlan'
dependencies = []
files = []
hgrepos = []
issue_num = 14019
keywords = ['patch']
message_count = 19.0
messages = ['153392', '153729', '154000', '154002', '194709', '195061', '196074', '217757', '217904', '217927', '218159', '218163', '218976', '219011', '219508', '348636', '380590', '380625', '380665']
nosy_count = 10.0
nosy_names = ['terry.reedy', 'ncoghlan', 'vstinner', 'eric.smith', 'ezio.melotti', 'eric.araujo', 'francismb', 'moijes12', 'francisco.freire', 'nanjekyejoannah']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14019'
versions = ['Python 3.10']

@ncoghlan
Copy link
Contributor Author

A couple of issues have arisen where features were added to str.format without similarly being added to string.Formatter.

This is only possible because the test cases for the two are currently almost entirely separate.

A common set of tests defined as (fmt, args, kwargs, output) tuples would help ensure that the implementations remained consistent in the future.

@ncoghlan ncoghlan added the tests Tests in the Lib/test dir label Feb 15, 2012
@merwok
Copy link
Member

merwok commented Feb 19, 2012

(ISTM that such test improvements would be beneficial to all branches, and we would also eschew merge issues if we change all branches. Other core devs sometimes object to test improvements or additions in stable branches though.)

@merwok merwok added the easy label Feb 19, 2012
@francismb
Copy link
Mannequin

francismb mannequin commented Feb 22, 2012

I have some questions about this:

  1. In Lib/test/string_tests.py it says:
    “Common tests shared by test_str, test_unicode, test_userstring and
    test_string”

but

a) I cannot find test_str

b) string is imported and only some constants ascii_letters
and digits are used

c) In test_join there is a comment “see the test in
test.test_string.StringTest.test_join” Is that obsolete?
(I cannot find StringTest in the test_string test)

  1. Is there more tests for the build in format (in
    test_buildin.BuiltinTest.test_format only the basic machinery is tested.
    I would expect something like in test_format.py somewhere

  2. it is true that all tests for the build in 'format' should also pass
    in 'string.Formatter().format'

Thanks in advance!

francis

@ncoghlan
Copy link
Contributor Author

At least the first couple of those look like obsolete comments left over from the 2.x branch (we didn't do a mass renaming in the test suite, so many tests still live in their old locations).

@franciscofreire
Copy link
Mannequin

franciscofreire mannequin commented Aug 8, 2013

I increased the coverage of formatter module to 40%. I added a new file "test_formatter.py" including some unit tests.

@ezio-melotti
Copy link
Member

I left a review on rietveld.

FWIW I think string_tests and related files should undergo a (major?) refactoring. I worked with them a few times and it's a bit of a mess with all those base classes and mixins. I also found some tests that weren't running because they were accidentally overridden by one of the subclasses, or other tests that were duplicated. This is especially true on Python 3, where bytes and str share a smaller subset of features.

@ezio-melotti ezio-melotti added the type-feature A feature request or enhancement label Aug 13, 2013
@franciscofreire
Copy link
Mannequin

franciscofreire mannequin commented Aug 24, 2013

Thanks for the review. I corrected some issues in my code. Here is the new patch.

@francismb
Copy link
Mannequin

francismb mannequin commented May 2, 2014

The formatter module was deprecated in Python 3.4 and is scheduled
for removal in Python 3.6. See [1] and [2].

---
[1] https://docs.python.org/3/library/formatter.html#module-formatter
[2] https://docs.python.org/3/whatsnew/3.4.html#deprecated

@brettcannon brettcannon self-assigned this May 3, 2014
@ncoghlan
Copy link
Contributor Author

ncoghlan commented May 5, 2014

Note that this issue wasn't about the formatter module - it relates to the str.format() method and the string.Formatter *class*.

The formatter module is completely unrelated.

@ericvsmith
Copy link
Member

The issue is about tests for str.format and string.Formatter, but in http://bugs.python.org/msg194709 and the associated patch there are tests for the formatter module to increase its coverage.

I suggested on the mentorship list that we break this into two issues.

@brettcannon
Copy link
Member

Francisco, can you sign the contributor agreement? https://www.python.org/psf/contrib/contrib-form/

@brettcannon
Copy link
Member

I added some review comments.

@brettcannon
Copy link
Member

Set to "pending" while I wait to hear back from Francisco on the review comments.

@franciscofreire
Copy link
Mannequin

franciscofreire mannequin commented May 23, 2014

Hi, I signed the contributor agreement. Thank you for your review comments. I did these tests about one year ago and right now I don't have much time to look at it again. I hope to do so in the next months.

@moijes12
Copy link
Mannequin

moijes12 mannequin commented Jun 1, 2014

Note that this issue wasn't about the formatter module - it relates to the str.format() method and the string.Formatter *class*.

I would tend to agree with Nick and Eric. From what I see in the patch, the tests are for formatter module and not the string.Formatter class.

@brettcannon brettcannon removed their assignment Apr 24, 2015
@vstinner
Copy link
Member

This issue is no newcomer friendly, I remove the "easy" keyword.

@vstinner vstinner removed the easy label Jul 29, 2019
@iritkatriel iritkatriel added 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels Nov 6, 2020
@vstinner
Copy link
Member

vstinner commented Nov 9, 2020

@Irit Katriel: Please avoid changing the versions field of old inactive issues. It changes artificially the last activity date of an issue and it sends an email to all people in the nosy list. There is no need to update the Version field of the +7500 open bugs.

@terryjreedy
Copy link
Member

Victor: Irit is reviewing old issues to decide whether to close or not. She has to touch it somehow to mark it as reviewed.

Irit: if you only change the version, others may think that you blindly updated the version. Better to say something that moves the issue forward. Also, only marking for the next version, now 3.10, would delay the possible future obsolescence of the version marking. In any case, any coredev who merges would decide about backports.

This particular issue needs major surgery as it is two interleaved but unrelated discussions. Francisco should have moved his unrelated patch to a new issue as Eric Smith asked. I opened new bpo-42299 and moved the revised patch there, with credit to Francisco and Ezio as reviewer. I unlinked both from here.

To make the discussion of Nick's issue more readable, the unrelated messages about the unrelated patch should be unlinked. I believe each message unlink would generate a separate email, as did the file unlinks.

@terryjreedy terryjreedy removed 3.8 only security fixes 3.9 only security fixes labels Nov 10, 2020
@vstinner
Copy link
Member

Since nobody implemented this idea in 8 years, maybe it's time to give up and close this issue as out of date. It seeems like Nick was busy with other stuff, and nobody took this task in the meanwhile.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@abrahammurciano
Copy link

abrahammurciano commented Apr 10, 2024

Hi, I know this is old, but I believe there's still significant value in unifying these tests. I stumbled across this difference between them in python3.12 and perhaps unifying the tests would have caught it.

>>> "{[0]}".format([1])
'1'
>>> Formatter().format("{[0]}", [1])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "***/python/3.12/lib/python3.12/string.py", line 190, in format
    return self.vformat(format_string, args, kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "***/python/3.12/lib/python3.12/string.py", line 194, in vformat
    result, _ = self._vformat(format_string, args, kwargs, used_args, 2)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "***/python/3.12/lib/python3.12/string.py", line 234, in _vformat
    obj, arg_used = self.get_field(field_name, args, kwargs)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "***/python/3.12/lib/python3.12/string.py", line 299, in get_field
    obj = self.get_value(first, args, kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "***/python/3.12/lib/python3.12/string.py", line 256, in get_value
    return kwargs[key]
           ~~~~~~^^^^^
KeyError: ''

Edit: I see that this bug is already reported here: #71494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 only security fixes tests Tests in the Lib/test dir type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

9 participants