-
Notifications
You must be signed in to change notification settings - Fork 0
Add libguides and researchdatabases aliases #324
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
Conversation
ghukill
left a comment
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.
Change to the alias part looks good!
I did leave a comment about the update to the OAI-PMH harvester CLI comamnd preparation.
| INDEX_ALIASES: ClassVar = { | ||
| "rdi": ["jpal", "whoas", "zenodo"], | ||
| "timdex": ["alma", "aspace", "dspace"], | ||
| "timdex": ["alma", "aspace", "dspace", "libguides", "researchdatabases"], |
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.
👍
lambdas/commands.py
Outdated
| ) | ||
| extract_command.append("harvest") | ||
| if source in ["aspace", "dspace"]: | ||
| if source in ["aspace", "dspace", "libguides", "researchdatabases"]: |
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.
Do we need this change here? I think this --method=get CLI argument for the OAI harvester will have it use the non-conventional GET request per method: https://github.com/MITLibraries/oai-pmh-harvester?tab=readme-ov-file#oai-harvest.
To my knowledge both libguides and researchdatabases have been harvesting fine without that.
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.
Ah, good catch, I'll remove. Thanks!
|
Fixed with the new commit @ghukill! |
Pull Request Test Coverage Report for Build 17679724162Details
💛 - Coveralls |
ghukill
left a comment
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.
Approved!
Purpose and background context
Adding
libguidesandresearchdatabasesaliases to get them into the TIMDEX feedHow can a reviewer manually see the effects of these changes?
NA
Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)