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 Journal to Citation model type #4776

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

flooie
Copy link
Contributor

@flooie flooie commented Dec 3, 2024

Add Journal as type of citation in the model

We added journals to eyecite, but it turns out many of the parallel citations are are in fact
journals. This distinction can be found in eyecite so it is worth bringing that distinction over
to CL.

This change is needed to resolve #4775.

The alternative was to map journals to specialty reporters - but that did not seem correct.

Add citation type journal which can be either a
scholarly publication or trade publication
Copy link

sentry-io bot commented Dec 3, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: cl/citations/utils.py

Function Unhandled Issue
map_reporter_db_cite_type KeyError: 'journal' cl.citations.utils in map_rep...
Event Count: 1
📄 File: cl/corpus_importer/management/commands/update_casenames_wl_dataset.py (Click to Expand)
Function Unhandled Issue
parse_citations KeyError: 'journal' cl.citations.utils in map_rep...
Event Count: 1
---

Did you find this useful? React with a 👍 or 👎

Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

One comment so far. It'd be nice to see a test here too, please, then maybe somebody from the case law team can review?

Comment on lines 143 to 145
if len(found_cites) == 0:
logger.info("Unable to parse %s", cite_str)
continue
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the same as line 141?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sorry let me clean this up .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I take that back. That’s not what I meant to write. The issue here is that we are correctly finding one citation when we pass in a single string citation, which is expected. However, the problem arises because the function identifies two editions for this citation: one journal and one reporter. We don’t want to arbitrarily choose the first one

@mlissner mlissner removed their assignment Dec 4, 2024
@flooie flooie assigned mlissner and unassigned mlissner Dec 4, 2024
Copy link
Member

@quevon24 quevon24 left a comment

Choose a reason for hiding this comment

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

everything looks good

@flooie
Copy link
Contributor Author

flooie commented Dec 4, 2024

@mlissner looks like we are good to go - but there is a noop migration so I wouldn't merge this without your approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

KeyError: 'journal'
3 participants