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

Add merging of names in last-name-first style #422

Merged
merged 23 commits into from
Jan 18, 2024
Merged

Conversation

mlutze
Copy link
Contributor

@mlutze mlutze commented Nov 23, 2023

I added a flag to MergeNameParts so the user can decide whether to use first-name-first or last-name-first.
I made last-name-first the default.

I slightly modified one existing test group to support the flag.

I added a new test group test_merge_last_name_first_inverse which ensures that merging with commas is the inverse of splitting. I skip this test for cases including a /, since this property doesn't hold for some of those cases.

I also had to do a bit of strange stuff with the typing imports. The flag I added is a union of literal strings, but literal string types aren't available in Python 3.7. So I try importing a literal type and if that doesn't work then I just say the type of the flag is "str", which is too general but the closest I can get

@MiWeiss
Copy link
Collaborator

MiWeiss commented Nov 27, 2023

Thanks for your work on this. Just to avoid a misunderstanding: I wont review this PR while it is marked as draft.

@mlutze
Copy link
Contributor Author

mlutze commented Nov 27, 2023

Thanks for your work on this. Just to avoid a misunderstanding: I wont review this PR while it is marked as draft.

Understood. There are still some changes to testing and documentation I'd like to make before submitting for review.

@mlutze mlutze marked this pull request as ready for review November 28, 2023 08:17
@mlutze
Copy link
Contributor Author

mlutze commented Dec 4, 2023

@MiWeiss This is ready for review.

Copy link
Collaborator

@MiWeiss MiWeiss left a comment

Choose a reason for hiding this comment

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

Hi @mlutze

Thanks a lot for opening th e PR. This is certainly a very important contribution.

I see (and acknowledge) that the whole \\ handling is a bit of a struggle. Still, I'd like to understand it a bit better to know whats going on. I hope you don't mind my corresponding review ;-)

@@ -7,6 +7,14 @@
import dataclasses
from typing import List, Tuple

# Literal is only available in Python 3.8+
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the meantime, python 3.7 has reached EOL.

I have just updated the CI on master to not use 3.7 anymore, thus feel free to remove your fallback.

tests/middleware_tests/test_names.py Outdated Show resolved Hide resolved
@tdegeus tdegeus mentioned this pull request Jan 18, 2024
2 tasks
@MiWeiss
Copy link
Collaborator

MiWeiss commented Jan 18, 2024

Thanks again @mlutze for your PR, and sorry that the review took so long!

@MiWeiss MiWeiss merged commit ccd1548 into sciunto-org:main Jan 18, 2024
18 checks passed
@MiWeiss MiWeiss mentioned this pull request Jan 18, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants