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 z3998:name-title semantic #614

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

erinendrei
Copy link
Contributor

@erinendrei erinendrei commented Oct 21, 2023

Issue #477 has been open for a while. If Robin still wants to do it, that's of course fine, but I thought I would try it as unless I'm missing something it doesn't look complicated.

  • In 6af0b22 the z3998:name-title semantic has been added to every abbreviation in the semanticate function that looked like a relevant title. Notes:

    • "Mt." was not given the semantic as it seems to abbreviate only "Mount"/"Mountain" even though it's been placed in the middle of the list of all the other titles.
    • "Bros." was not given the semantic as it doesn't seem like a title (unless, I guess, it's referring to a group of religious brothers, for whom "Br." is actually a title; but this seems like it would occur less frequently than other possible uses of "Bros.").
    • "M." for "Monsieur", mentioned by Robin initially, does not appear to be automatically given <abbr> by semanticate and was not added here as I figured that that omission was intentional, perhaps because "M." could be part of an initialism or given name.

    If I'm mistaken in any choices here, let me know and I'll rebase.

  • In dac0b89 the test for semanticate has been updated to accord with the changes in 6af0b22. If you want more test cases for z3998:name-title specifically, then I can add some. It seems like the one for "Mr." at line 12 of tests/semanticate/in/semanticate.xhtml was previously thought to be enough.

  • In 433327f the other tests containing "Mr." and "Dr." were updated with the z3998:name-title semantic. I didn't see other title abbreviations, and force-pushed because I missed "Dr." the first time.

  • There is no lint check here. For abbreviations that need the z3998:name-title semantic but lack it, would the correct error code be s-045 or would a new error code be required? I could add a lint check and test in a later PR or update this one if you want.

Even if the commits here are acceptable I obviously can't update the ebook repos as mentioned originally in #477 so this addresses only part of the issue and doesn't close it. And I imagine the manual would have to be updated too if you still want to accept this semantic change.

@acabal
Copy link
Member

acabal commented Oct 24, 2023

Great, thanks. The trickier part is updating the corpus. Can you write a Bash script for updating a set of repos locally? For example, if we have all of the repos as folders on a local machine, loop through them and use sed or something to make these replacements. You can email me that, instead of putting it in this PR. Then we can accept this since we can update the corpus at the same time.

You can use the sync-ebooks script to download the corpus locally to test if you want.

@erinendrei
Copy link
Contributor Author

Sure, I'll work on that. Thanks.

@erinendrei
Copy link
Contributor Author

Did you receive the email last week? No rush if so, but I realised the attachment might have sent it to spam.

@acabal
Copy link
Member

acabal commented Oct 30, 2023

Yes, I am unbelievably busy so this will probably have to wait till next week

@acabal acabal merged commit 4b7035e into standardebooks:master Nov 28, 2023
1 check passed
@acabal
Copy link
Member

acabal commented Nov 28, 2023

OK, I've had time to look this over and it looks great. Thanks!

@erinendrei erinendrei deleted the add-name-title branch November 30, 2023 01:16
acabal pushed a commit to standardebooks/manual that referenced this pull request Mar 25, 2024
acabal pushed a commit to standardebooks/manual that referenced this pull request Apr 16, 2024
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.

2 participants