Skip to content

Fix for Github issue 98#99

Merged
ehenneken merged 2 commits into
adsabs:masterfrom
ehenneken:github_issue_98
Apr 20, 2026
Merged

Fix for Github issue 98#99
ehenneken merged 2 commits into
adsabs:masterfrom
ehenneken:github_issue_98

Conversation

@ehenneken
Copy link
Copy Markdown
Member

This PR provides a fix for #98 . Since the exception also occurs on the /parse endpoint, this PR also provides a solution for this case. In the case of the /parse endpoint, the output has been augmented to include a rejected entry in the JSON that will list all reference strings that could not be processed (because of an exception). The crf.py module has been updated to throw an exception with a specific error text to indicate that it was the collection of tagged token that threw the exception.

Unittests have been added to cover the changes.

@ehenneken ehenneken added the bug Something isn't working label Apr 14, 2026
Copy link
Copy Markdown

@Thomas-S-Allen Thomas-S-Allen left a comment

Choose a reason for hiding this comment

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

Line 463 in referencesrv/views.py could append an empty list to the response. Could replace with:

response = {'parsed': results}
if rejected:
    response['rejected'] = rejected

Note that this would require line 819 in referencesrv/tests/unittests/test_referencesrv_parser.py to revert back to its previous state.

Line 833 in referencesrv/tests/unittests/test_referencesrv_parser.py: replace with with mock.patch.object(self.current_app.client, 'get') as get_mock: because it is mocking the SOLR call - solrquery.py line 55 uses response = client().get(...)

Line 315 in referencesrv/parser/crf.py: can make the exception causality chain more explicit by changing raise Exception('Failed to generate tagged tokens: {0}'.format(err)) to raise Exception('Failed to generate tagged tokens: {0}'.format(err)) from err

Also, for .gitignore should .DS_Store be kept for Mac users?

For the unit tests, can further ensure a graceful response by adding self.assertEqual(r.status_code, 200) before line 827 and 842

There are a couple of pre-existing items, the first is a bug:

Line 229 in referencesrv/views.py - raise "Not Resolved" should raise an exception object not a string: raise ValueError("Not Resolved")

Lines 201-216 have duplicate exception blocks, the first excepts specific errors, the second is generic

Copy link
Copy Markdown

@Thomas-S-Allen Thomas-S-Allen left a comment

Choose a reason for hiding this comment

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

Looks good to merge.

@ehenneken ehenneken merged commit 1be670d into adsabs:master Apr 20, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants