DataONEorg / scythe

Scythe, the data citation harvester
Other
7 stars 2 forks source link

refactor scythe_set_key to not hardcode key names #17

Closed mbjones closed 3 years ago

mbjones commented 3 years ago

scythe_set_key currently hardcodes key names as parameters to the function, which will require changes to the function signature when we want to add new sources. Refactor to:

As this is a signature change, I think we should add it to the 0.9.0 release so we don't establish a function we later need to deprecate.

jeanetteclark commented 3 years ago

I did this refactor, but the function does throw an error if the source is not named either scopus or springer. The rest of the functions need to know how to look for keys, and since this function is meant to be a helper on top of the keyring functions, it seemed reasonable to me. I could allow for arbitrary names I guess, and users could pass those to the relevant functions. eg:

scythe_set_key(source = "foo", secret = "bar")
citation_search_scopus(identifiers, key_name = "foo")

But I'm not sure what problem this solves exactly, other than giving users more ways to trip over setting up keys correctly. Since the keys don't expire, I think a "set it and forget it" approach would be more convenient. Setting the default argument for key_name in the above to "scopus", and giving a warning on scythe_set_key if keys are set with different names might help, but again, I'm not totally sure what the problem we would be solving is.

I could add the keyring parameter here, and users could pass that into the citation_search functions, which would enable users to set a different keyring name. Similar to above though, I'm not sure what problem this solves, other than if for some reason someone already has a keyring named scythe (there is already a warning for this case, anyway).

mbjones commented 3 years ago

Thanks @jeanetteclark The main problem I was anticipating is supporting multiple and possibly a much larger number of sources. Your solution fixes that. I am envisioning that, rather than 3 sources, we could get to a point where we have dozens. So, I was trying as a design goal to avoid hardcoding source names in the function arguments that makes them harder to maintain. I think what you did is fine, though, given that the signature no longer hardcodes parameters for each key name. If we want to remove your check later to support more sources, we can do it without breaking client code now, so it would be backwards compatible. So, I am going to close this for now.