Skip to content

Don't return an empty publisher from get_location_and_publisher - #14149

Merged
hornc merged 8 commits into
internetarchive:masterfrom
Kiddo4993:fix/issue-14140
Nov 3, 2026
Merged

Don't return an empty publisher from get_location_and_publisher#14149
hornc merged 8 commits into
internetarchive:masterfrom
Kiddo4993:fix/issue-14140

Conversation

@Kiddo4993

@Kiddo4993 Kiddo4993 commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #14140.

get_location_and_publisher() strips out the Place of publication not identified marker. When that marker was the whole input, the fallback branch still wrapped the now-empty string in a list and returned ([], [""]). The import API only checks whether the publisher list is non-empty, so this empty string could end up in edition metadata as publishers=[""].

The fallback now returns an empty publisher list when the stripped value is empty. The colon-separated branch already filters out empty items, so this makes the two paths behave the same way.

Tests: I added cases to test_get_location_and_publisher for "Place of publication not identified" and "[Place of publication not identified]". Both should return ([], []). I couldn't run the tests in this environment.

Fixes #14140

Happy to adjust anything or close it if it isn't a fit.

Copilot AI balanced review requested due to automatic review settings October 28, 2026 04:17
Copilot AI reviewed Oct 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@openlibrary-bot

openlibrary-bot commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @Kiddo4993 — and welcome, this looks like your first contribution to Open Library! 🎉

The linked issue hasn't been triaged yet — triage happens on Mondays and Fridays. There are currently 99 open non-draft PRs ahead of yours.

PR triage checklist (maintainers / Pierre)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains a #NNN reference
    • Linked issue is triaged — has a Priority: * label (not just Needs: Triage)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — no WIP/fixup/conflict noise; commit messages are meaningful
  • CI passing — no failing check-runs
  • Test cases present — if the change touches substantive logic, test coverage exists or is explained
  • Proof of testing — PR body includes a description of what was tested, a screenshot, or a video

Note

This comment was automatically generated by Pierre, Open Library's PR first-touch assistant, on behalf of @mekarpeles. This is NOT a code review or sign-off.

@RayBB RayBB mentioned this pull request Nov 3, 2026
Merged
@hornc hornc mentioned this pull request Nov 3, 2026
Closed
1 of 2 tasks
hornc added 2 commits November 4, 2026 10:18
@hornc
parametrize test_get_colon_only_loc_pub
4cb78b5
@hornc
refactor test with parametrize
31ba566 
add a populatued value with no colon publisher test to
properly test the second path
@hornc
hornc force-pushed the fix/issue-14140 branch 2 times, most recently from 6c0e7aa to 7066149 Compare November 3, 2026 22:22
@hornc
hornc merged commit 684bd64 into internetarchive:master Nov 3, 2026
4 of 5 checks passed
@RayBB RayBB mentioned this pull request Nov 3, 2026
Closed
4 tasks
RayBB added a commit that referenced this pull request Nov 3, 2026
@RayBB
Fix mypy: move type: ignore to the get_location_and_publisher call
ccc3978 
The ignore in test_get_location_and_publisher_bad_type sat on the list
assignment, but mypy reports the error on the call as [arg-type]. The
test only started being checked once #14149 added a -> None annotation,
so pre-commit.ci mypy has failed on master since then.
@RayBB RayBB mentioned this pull request Nov 3, 2026
Merged
RayBB added a commit that referenced this pull request Nov 3, 2026
@RayBB
Fix mypy: move type: ignore to the get_location_and_publisher call (#…
42885dd 
…14273)

The ignore in test_get_location_and_publisher_bad_type sat on the list
assignment, but mypy reports the error on the call as [arg-type]. The
test only started being checked once #14149 added a -> None annotation,
so pre-commit.ci mypy has failed on master since then.
marley pushed a commit to marley/openlibrary that referenced this pull request Nov 4, 2026
@Kiddo4993 @RayBB
@hornc @pre-commit-ci @marley
Don't return an empty publisher from get_location_and_publisher (inte…
b0dca0a 
…rnetarchive#14149)

* Don't return an empty publisher from get_location_and_publisher

Fixes internetarchive#14140

* refactor

* parametrize test_get_colon_only_loc_pub

* refactor test with parametrize

add a populatued value with no colon publisher test to
properly test the second path

* correct parametrize argument types

* type hint

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Raymond Berger <RayBB@users.noreply.github.com>
Co-authored-by: Charles Horn <charles.horn@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
marley pushed a commit to marley/openlibrary that referenced this pull request Nov 4, 2026
@Kiddo4993 @RayBB
@hornc @pre-commit-ci @marley
Don't return an empty publisher from get_location_and_publisher (inte…
bfd1153 
…rnetarchive#14149)

* Don't return an empty publisher from get_location_and_publisher

Fixes internetarchive#14140

* refactor

* parametrize test_get_colon_only_loc_pub

* refactor test with parametrize

add a populatued value with no colon publisher test to
properly test the second path

* correct parametrize argument types

* type hint

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Raymond Berger <RayBB@users.noreply.github.com>
Co-authored-by: Charles Horn <charles.horn@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
marley pushed a commit to marley/openlibrary that referenced this pull request Nov 4, 2026
@Kiddo4993 @RayBB
@hornc @pre-commit-ci @marley
Don't return an empty publisher from get_location_and_publisher (inte…
c3723be 
…rnetarchive#14149)

* Don't return an empty publisher from get_location_and_publisher

Fixes internetarchive#14140

* refactor

* parametrize test_get_colon_only_loc_pub

* refactor test with parametrize

add a populatued value with no colon publisher test to
properly test the second path

* correct parametrize argument types

* type hint

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Raymond Berger <RayBB@users.noreply.github.com>
Co-authored-by: Charles Horn <charles.horn@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
marley pushed a commit to marley/openlibrary that referenced this pull request Nov 4, 2026
@Kiddo4993 @RayBB
@hornc @pre-commit-ci @marley
Don't return an empty publisher from get_location_and_publisher (inte…
476419f 
…rnetarchive#14149)

* Don't return an empty publisher from get_location_and_publisher

Fixes internetarchive#14140

* refactor

* parametrize test_get_colon_only_loc_pub

* refactor test with parametrize

add a populatued value with no colon publisher test to
properly test the second path

* correct parametrize argument types

* type hint

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Raymond Berger <RayBB@users.noreply.github.com>
Co-authored-by: Charles Horn <charles.horn@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
marley pushed a commit to marley/openlibrary that referenced this pull request Nov 4, 2026
@Kiddo4993 @RayBB
@hornc @pre-commit-ci @marley
Don't return an empty publisher from get_location_and_publisher (inte…
8162ac9 
…rnetarchive#14149)

* Don't return an empty publisher from get_location_and_publisher

Fixes internetarchive#14140

* refactor

* parametrize test_get_colon_only_loc_pub

* refactor test with parametrize

add a populatued value with no colon publisher test to
properly test the second path

* correct parametrize argument types

* type hint

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Raymond Berger <RayBB@users.noreply.github.com>
Co-authored-by: Charles Horn <charles.horn@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@openlibrary-bot openlibrary-bot Awaiting requested review from openlibrary-bot

Assignees

@hornc hornc

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Publication parser returns an empty publisher after removing the unidentified-place marker

5 participants

@Kiddo4993 @openlibrary-bot @hornc @RayBB