Skip to content

GH-90829: Fix empty iterable error message in min/max #31181

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

Merged

Conversation

Nnarol
Copy link
Contributor

@Nnarol Nnarol commented Feb 7, 2022

Change misleading "empty sequence" to "empty iterable" in builtins.min/max.
Update corresponding test to be more specific and match the error string.

https://bugs.python.org/issue46671

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@invalid-email-address

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

CLA Missing

Our records indicate the following people have not signed the CLA:

@Nnarol

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@Nnarol Nnarol force-pushed the fix-min-max-empty-it-error-msg-46671 branch from 5321ba4 to 581c5a9 Compare February 7, 2022 00:49
@Nnarol
Copy link
Contributor Author

Nnarol commented Feb 7, 2022

I am curious as to the @invalid-email-address issue. I definitely have a bpo account with my GitHub user name, Nnarol associated.
The e-mail address and name when committing from my local machine do not match that. Could that be the problem? Still, that has nothing to do with GitHub.

Another thing that confuses me is why patchcheck automatically changed Tools/scripts/freeze_modules.py and Lib/test/support/init.py .

The third thing: is Windows essentially really required to contribute? The Adobe sign thingy used for signing the CLA says either Windows or Mac OS.

Thanks in advance to anyone who can help!

@sweeneyde
Copy link
Member

I haven't reviewed this closely, but I agree with the basic idea. The phrasing "empty sequence" is 32 years old! It predates the iterator protocol, so it makes sense to change since iterators are expected.

err_setstr(RuntimeError, "min() or max() of empty sequence");

It looks like this PR is currently set to changes some files (freeze_modules.py and support/__init__.py) unrelated to the central change. It would be preferable to revert those so the PR is more focused and the git history is more clear.

@Nnarol Nnarol force-pushed the fix-min-max-empty-it-error-msg-46671 branch from e819301 to 4637e0f Compare February 7, 2022 01:29
@Nnarol
Copy link
Contributor Author

Nnarol commented Feb 7, 2022

@sweeneyde Thanks for your reply! At least I know that the changes to those 2 files patchcheck made aren't necessary and it's safe to revert them. Done.
In the meanwhile, I have also managed to fill out the CLA through pbo rather than python.org. I wonder if the bot's message will ever update.
I am still confused about the not-found bpo account.

Change misleading "empty sequence" to "empty iterable" in builtins.min/max.
Update corresponding test to be more specific and match the error string.
@Nnarol Nnarol force-pushed the fix-min-max-empty-it-error-msg-46671 branch from 4637e0f to 6802cfc Compare February 7, 2022 07:25
…71.CmpNlQ.rst


Spelling fix in RST documentation.

Co-authored-by: Nikita Sobolev <[email protected]>
@Nnarol
Copy link
Contributor Author

Nnarol commented Feb 8, 2022

@sobolevn Thanks!

@ghost
Copy link

ghost commented May 2, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I think you'll have to resign the CLA under our new procedure, sorry for that!

@Nnarol
Copy link
Contributor Author

Nnarol commented May 3, 2022

@JelleZijlstra Under the following related issue:

#91559

another core developer, @rhettinger deemed the change not readable, and the issue was closed. Are you sure this one should proceed?

@JelleZijlstra
Copy link
Member

Raymond doesn't have a monopoly on approving or rejecting changes, and on this thread @sweeneyde also agreed with the premise of this change. I'll leave this open for a little while to give others a chance to provide feedback or suggest better wording.

No news entries are needed for minor docs fixes.
@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@Nnarol
Copy link
Contributor Author

Nnarol commented May 7, 2022

Hi @JelleZijlstra and @rhettinger ! Since the RST file was deleted as it is unnecessary for documentation changes, the respective automated check now fails. I assume this does not technically prevent a merge, right?

@rhettinger
Copy link
Contributor

rhettinger commented May 10, 2022

I didn't have a strong concern on this one, so go ahead and proceed :-)

Normally though, if a core developer objects to a change, there needs to be a discussion rather than trampling their concern. This is especially true for a senior dev who is more familiar the norms and history of the project. This also helps newer devs learn how to handle nuance and establishes a better community norm than "I got someone else to give a thumbs up, so I can just ignore you." Also, we tend to place more weight on no votes so that if a change is questionable, the status quo wins.

The history of this particular issue is that sometimes we have elected to use the word sequence as general understandable English that than iterable which is more type specific. Python isn't just used by typing experts, it is also used by kids, high school students, and non-IT professionals.

Overtime the word iterable have become better understood by day-to-day Python users, so the bar is much lower. Still, in some places we still save "sequence (or other iterable)" for clarity.

Since this is just an error message, changing it is fine.

@Nnarol
Copy link
Contributor Author

Nnarol commented May 11, 2022

@rhettinger As a new developer, I find your reasoning very sensible, which is why I mentioned your concerns here.

[...] we have elected to use the word sequence as general understandable English that than iterable which is more type specific.

In fact, to my knowledge, there is a general term that corresponds to the more type system-specific iterable, namely collection, which I've seen in various contexts involving Python and does not express anything about ordering, as opposed to sequence.

EDIT: Thinking back, however, I think I have never seen "collection" used in actual Python code or error messages.

@JelleZijlstra
Copy link
Member

I didn't have a strong concern on this one, so go ahead and proceed :-)

Normally though, if a core developer objects to a change, there needs to be a discussion rather than trampling their concern. This is especially true for a senior dev who is more familiar the norms and history of the project. This also helps newer devs learn how to handle nuance and establishes a better community norm than "I got someone else to give a thumbs up, so I can just ignore you." Also, we tend to place more weight on no votes so that if a change is questionable, the status quo wins.

I should have worded my previous message better, but I didn't merge the PR or express an intent to merge it, so that's hardly "trampling". I do agree that we should default to the status quo if there's no clear consensus, but I feel strongly that we should give the discussion a chance to run its course before closing attempts to fix a perceived problem. So let's go back to discussing the best wording for this error message.

The history of this particular issue is that sometimes we have elected to use the word sequence as general understandable English that than iterable which is more type specific. Python isn't just used by typing experts, it is also used by kids, high school students, and non-IT professionals.

Overtime the word iterable have become better understood by day-to-day Python users, so the bar is much lower. Still, in some places we still save "sequence (or other iterable)" for clarity.

Since this is just an error message, changing it is fine.

I do see the concern, but "iterable" is already pervasive in error messages (for example, list(3) gives 'int' object is not iterable), so it's likely users will already encounter the term. I'd generally advocate that we use terms in the precise meanings established by the collections.abc classes. That doesn't mean users necessarily need to understand the precise meaning of the term, but users who want to understand the meaning of the term can look it up in the glossary. Using terms in a different meaning than the ABCs can lead to user confusion.

In this case, "iterable" is the right ABC to use. @Nnarol suggested "collection", but the Collection ABC requires __len__ and __contains__ in addition to __iter__, so it's not appropriate to mention it here.

@JelleZijlstra JelleZijlstra removed their assignment Jul 6, 2022
@kumaraditya303 kumaraditya303 changed the title bpo-46671 Fix empty iter error message in min/max GH-90829: Fix empty iterable error message in min/max Jan 8, 2023
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, iterable term is better as it aligns with the ABCs and existing error messages.

>>> tuple(42) 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'int' object is not iterable
>>> 

@kumaraditya303 kumaraditya303 merged commit 0741da8 into python:main Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants