Incorrect isinstance(..., 'str') check, test _pyproject_toml

[maurycy]

'str' is incorrect there:

>>> isinstance('meow', 'str')
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    isinstance('meow', 'str')
    ~~~~~~~~~~^^^^^^^^^^^^^^^
TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union
>>> isinstance('meow', str)
True

Reference:

• https://docs.python.org/3/library/functions.html#isinstance

[maurycy]

If I find more similarly obvious issues, I will create one PR for them to avoid flooding the PR here.

[hugovk]

Looks like we're missing tests for this bit of code. Would it be possible to add something?

[maurycy]

@hugovk

Looks like we're missing tests for this bit of code. Would it be possible to add something?

Definitely! Thank you for spotting this.

What do you think about bd5e2d2? It fails on the main branch.

Personally, I'm not a huge fan of this, as it's importing an _internal module. It seems to be the rule, though:

• https://github.com/python/pyperformance/blob/main/pyperformance/tests/test_python.py#L4
• https://github.com/python/pyperformance/blob/main/pyperformance/tests/test_pythoninfo.py#L7
• https://github.com/python/pyperformance/blob/main/pyperformance/tests/test_venv.py#L8

If you like this stub, I can extend it with more tests, especially given that #109 wasn't a particularly lucky PR:

• https://github.com/python/pyperformance/pull/109/files#diff-3e8f1cf61bbb100da8f1596584dbbfbdab1381669a6c4a036246bd19b6eea3c7R191-R193 (https://github.com/python/pyperformance/blob/main/pyperformance/_pyproject_toml.py#L201-L203)
• https://github.com/python/pyperformance/pull/109/files#diff-3e8f1cf61bbb100da8f1596584dbbfbdab1381669a6c4a036246bd19b6eea3c7R248-R251 (https://github.com/python/pyperformance/blob/main/pyperformance/_pyproject_toml.py#L259-L261)
• https://github.com/python/pyperformance/pull/109/files#diff-37e368652aee97ec2843ef7927faa34e8e5d681315643af43890e02b8448868bR31 (https://github.com/python/pyperformance/blob/main/pyperformance/_utils.py#L34)
• https://github.com/python/pyperformance/pull/109/files#diff-71569863b6c2ec4af655ac7c8a3a9af1e12f7a4ee2d6c571d74f696c880949c8R115 (https://github.com/python/pyperformance/blob/main/pyperformance/_benchmark_metadata.py#L116)
• https://github.com/python/pyperformance/pull/109/files#diff-3e8f1cf61bbb100da8f1596584dbbfbdab1381669a6c4a036246bd19b6eea3c7R29-R36 (https://github.com/python/pyperformance/blob/main/pyperformance/_pyproject_toml.py#L32-L39)

More generally, one could wonder if the project should reimplement PEP 621. Are we OK with exploring https://github.com/pypa/pyproject-metadata? I wish we uv in Python projects. ;-)

More importantly: how much of this code was ever executed, given no bug reports over the past 3 years? Similarly to how we wondered after running ruff check. Notably: ValuError also happened in pyperformance/_pyproject_toml.py.

[maurycy]

https://docs.python.org/3/library/textwrap.html#textwrap.dedent

🤔

[hugovk]

Personally, I'm not a huge fan of this, as it's importing an _internal module. It seems to be the rule, though:

I think that's fine here.

More generally, one could wonder if the project should reimplement PEP 621. Are we OK with exploring pypa/pyproject-metadata?

Yeah, given the comment at the top of _pyproject_toml.py, I reckon using pypa/pyproject-metadata is ok:

pyperformance/pyperformance/_pyproject_toml.py

Lines 1 to 2 in 553aa5d

	# This module should be replaced with the equivalent functionality
	# in the PyPI "packaging" package (once it's added there).

[hugovk]

Thanks!