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

Strip RIGHT-TO-LEFT OVERRIDE and LEFT-TO-RIGHT OVERRIDE characters from AUTHORS.txt #12517

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Feb 8, 2024

Since we don't know anything about the names, we assume anything in between RIGHT-TO-LEFT OVERRIDE and LEFT-TO-RIGHT OVERRIDE (or the end of the name) should be spelled backwards.

This resulted in a duplicate author name,
because it uses different Unicode form.
So I also added a call to unicodedata.normalize.

Fixes #12467

@hroncok
Copy link
Contributor Author

hroncok commented Feb 8, 2024

This is marked draft because it has no changelog entry -- I suppose it does not need one.

Happy to receive feedback.

…om AUTHORS.txt

Since we don't know anything about the names, we assume anything in between
RIGHT-TO-LEFT OVERRIDE and LEFT-TO-RIGHT OVERRIDE (or the end of the name)
should be spelled backwards.

This resulted in a duplicate author name,
because it uses different Unicode form.
So I also added a call to unicodedata.normalize.

Fixes pypa#12467
@DiddiLeija DiddiLeija added the skip news Does not need a NEWS file entry (eg: trivial changes) label Feb 8, 2024
@pradyunsg
Copy link
Member

Feel welcome to mark this ready for review!

@hroncok hroncok marked this pull request as ready for review February 8, 2024 23:36
Copy link
Member

@DiddiLeija DiddiLeija left a comment

Choose a reason for hiding this comment

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

I'd say we can merge this PR now, unless I am missing something?

@webknjaz
Copy link
Member

webknjaz commented Mar 3, 2024

Possible improvement: It might be useful to put the conversion into the pre-commit config. For example, we have author sorting on that level and it's nice that it can be autofixed: https://github.com/aio-libs/aiohttp/blob/0e91eb0/.pre-commit-config.yaml#L79.

@uranusjr
Copy link
Member

uranusjr commented Mar 3, 2024

+1 to having a pre-commit hook for this.

@hroncok
Copy link
Contributor Author

hroncok commented Mar 3, 2024

I am a bit lost. A pre-commit hook to do what exactly?

Instead of generating the AUTHORS file as we desire, we would generate it with the RTL OVERRIDE characters, and only then would we fix it by a pre-commit hook? How is that better?

@webknjaz
Copy link
Member

webknjaz commented Mar 3, 2024

I thought this file is also modified externally. I didn't realize you're modifying the script that produces it in the first place. I suppose your solution is correct, then.

@uranusjr
Copy link
Member

Gonna merge and see what happens.

@uranusjr uranusjr merged commit 7673a51 into pypa:main Mar 26, 2024
12 of 13 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RIGHT-TO-LEFT OVERRIDE in AUTHORS.txt
5 participants