-
Notifications
You must be signed in to change notification settings - Fork 1
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
SFR-2105: Deprecate OCLC Classify Manager #375
Conversation
|
||
mockCheckRedis.assert_called_once_with('classifyWork', '1', 'owi') | ||
def test_get_queryable_identifiers(self): | ||
assert ClassifyProcess._get_queryable_identifiers(['1|isbn', '2|test']) == ['1|isbn'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: newline at the end of this test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Once you're happy with the e2e processes, let's merge!
processes/oclcClassify.py
Outdated
return self.checkSetRedis('classifyWork', workOWI, 'owi') | ||
def _get_queryable_identifiers(identifiers): | ||
return list(filter( | ||
lambda x: re.search(r'\|(?:isbn|issn|oclc)$', x) != None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: x is very generic, we could use identifier or id to be specific.
Some functions were unused, but represent logic that was recreated in the Classify process (e.g. the "v2" metadata fetch function) and would benefit from new tests. Other portions of the removed logic seem to already be covered by new mapping tests.