The by_statement indicates that this is a translator, who probably shouldn't be promoted to author, but I'm not sure how this case can be detected without natural language processing of the by statement, so OK as is.

Standard MARC relator is "com". Is this an OpenLibrary specific code or are these not being normalized?

To answer my own question, this is coming from $e relator term, not the $4 relationship which uses standard codes, so it is basically free form text, loosely codified. Given that, the best we can do is preserve the information to be sorted out later. We should not strip trailing periods (full stops) here, because they indicate an abbreviated term, not the end of the field.

I think the standard relator for this might be "att" for "Attributed name"

Ignore the above comment for the purposes of this PR.

@hornc, I brought this up at the weekly ABC call and as Drini mentioned removing mypy is okay here, but we'd like to keep running the doctests.

@scottbarnes the majority of the tests run in source scripts/run_doctests.sh are already running in make test-py

Running them twice does not add value, it simply takes longer and gives a false impression of 'doing more testing'. It also makes it harder to debug and locate when a test fails if it is failing in two locations.

In general, I don't think the OL project's doctests are maintained or current. The majority of the tests run in /run_doctests.sh are not doctests anyway.

The correct place to put OL Python tests are in the standard test files to be run via make test-py

I believe that this has already been done, but it's hard to review since the bulk of what runs is simple duplication.

The ignore list in scripts/run_doctests.sh seems totally arbitrary. This script should be deprecated, and if there is anything still of value in doctests, they should be highlighted and moved to more appropriate locations, so they can be maintained properly.

Ah, thank you for pointing out that the they're already running from make test-py. I will try to look at this more closely tomorrow.

@hornc, I created #9929 to address this. We will discuss this during milestone planning, and I will mention that I think that issue is blocking this PR, which is itself blocking another PR, if I understand correctly.

I'm excited for this PR to land, the mypy changes and test + workflow changes shouldn't be required for it to land. Making the frustrating call to separate these two issues (as @scottbarnes has already been doing). The changes in this file should be reverted, we can test with make test-py inside the container. If there's a separate PR we need to address test cleanup, we can talk during community call when all stakeholders are available and all are welcome to join.

@scottbarnes has offered to fix the mypy change if necessary (which seems to be a lighter lift than doing all the test reconciliation) -- ty!

@mekarpeles @scottbarnes I've reinstated the doctests, the overall tests still pass, which is as expected. This should now be ready for final review and merge.

As stated above, there is a follow-up PR to fill out the contributor role abbreviations as a stand-alone feature / improvement: #9901

This is an improvement, but still not correct. I don't want to hold up this PR for it, but we should create an issue to correctly handle 710$t like this entry (perhaps dropping it is better for the time being?):

  <datafield tag="710" ind1="2" ind2=" ">
    <subfield code="a">Catholic Church.</subfield>
    <subfield code="b">Pope (1846-1878 : Pius IX).</subfield>
    <subfield code="t">Syllabus errorum (8 Dec. 1864).</subfield>
    <subfield code="l">English.</subfield>
  </datafield>

I'm not sure what to do here. We are basically only using this field as a source of 'organisation responsibility' so are deliberately ignoring work specific metadata subfields like $t - Title of a work , and just extracting enough to name the organisation responsible. It's not a perfect alignment, but in practice it seems to work when there is no other attribution for 'author'

To answer my own question, this is coming from $e relator term, not the $4 relationship which uses standard codes, so it is basically free form text, loosely codified. Given that, the best we can do is preserve the information to be sorted out later. We should not strip trailing periods (full stops) here, because they indicate an abbreviated term, not the end of the field.

Ignore the above comment for the purposes of this PR.

$e subfield needs to be treated specially here because name_from_list strips trailing periods which we don't want.

@tfmorris thanks for picking this up... I've changed it so that trailing dots are no longer stripped from $e / role, but that does leave "supposed author." with the trailing dot in that one example.

This seems to be the lesser of two evils, and I'm pleasantly surprised that we even have test cases that cover both sentence terminating points and abbreviation points. Abbreviated roles do seem common.

I'm not sure if we can rely on strict ISBD, AACR, or any other punctuation rules to make a correct distinction here. It looks like hybrid / ad-hoc cataloguing punctuation is to be expected in MARC. Unfortunately.

@tfmorris @scottbarnes I believe I've addressed the recent comments and pushed improvements. Is this good to merge now?

This looks good to me. Thanks for all the work on this @hornc and @tfmorris. I am going to merge this and if there is more to do I vote it is on me to fix it. :)