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

Fixed #35443 -- Changed ordinal to return negative integers as is. #18172

Merged
merged 1 commit into from
May 27, 2024

Conversation

S-Tornqvist
Copy link
Contributor

@S-Tornqvist S-Tornqvist commented May 16, 2024

Trac ticket number

ticket-35443

Branch description

"-1st", "-2nd" and so on are incorrect words. Function ordinal in module humanize has been changed to return negative number "as is", so that for example -1 is converted to "-1".

The changes corresponds to alternative 1 of the suggested solutions in ticket #35443, which was accepted by Sarah Boyce.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@Siburg
Copy link
Contributor

Siburg commented May 16, 2024

You beat me to it. I like the test with -0. As per the first comment on the ticket, you may want to tweak docs/ref/contrib/humanize.txt as well.

@S-Tornqvist
Copy link
Contributor Author

You beat me to it. I like the test with -0. As per the first comment on the ticket, you may want to tweak docs/ref/contrib/humanize.txt as well.

Thanks :)

I committed a fixup (8130b21) for the docs. Unsure what the policy for this is, but I assume everything is squashed when accepted? Didn't want to alter the history with a force push.

Copy link

@Robinrswanson Robinrswanson left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I’ve reviewed the code, tests, and documentation, and they all appear to follow the guidelines. However, for the pull request itself, the commits need to be squashed into a single commit. Once that’s done, it can proceed with the review.

@S-Tornqvist
Copy link
Contributor Author

Thank you for your contribution! I’ve reviewed the code, tests, and documentation, and they all appear to follow the guidelines. However, for the pull request itself, the commits need to be squashed into a single commit. Once that’s done, it can proceed with the review.

Great! The commits are now squashed. Thanks for the help.

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you!
I think I would update to only support "positive" integers as I think it's clearer than "non-negative" and "0th" also doesn't make sense in natural language 👍

docs/ref/contrib/humanize.txt Outdated Show resolved Hide resolved
tests/humanize_tests/tests.py Show resolved Hide resolved
Previously, `-1` was converted to `"-1th"`. This has been updated to
return negative numbers "as is", so that for example `-1` is
converted to `"-1"`. This is now explicit in the docs.

Co-authored-by: Martin Jonson <artin.onson@gmail.com>
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you @S-Tornqvist

@sarahboyce sarahboyce merged commit d3a7ed5 into django:main May 27, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants