altmetric / identifiers

Collection of utilities related to the extraction, validation and normalization of various scholarly identifiers.
https://rubygems.org/gems/identifiers
MIT License
5 stars 1 forks source link

Consistantly support nil or non-string values #14

Closed shamess closed 7 years ago

shamess commented 7 years ago

Fixes issues caused by attempting to extract identifiers on nil, or other non-string, values.

The precendent here is set by the Identifiers::DOI extract method, which already does this. It's rather handy to have nil values be supported as it removes the need for guards around this code.

jbilbo commented 7 years ago

Could you do the .to_s only to the .extract method? Instead of doing it in for example extract_post_2007_arxiv_ids and others. They're class methods and they're not private right now but they will, eventually. I was thinking on adding constructors so we could add useful things to identifiers like normalisation (kind of what we do in the urn gem). The current contract is the .extract method and nothing else, if someone is using other methods they'll have problems in the future, so better let it crash for these cases.

mudge commented 7 years ago

@jbilbo I think having the to_s closest to where it is genuinely needed makes sense in this case: after all, extract just calls out to the two, more specific (and potentially private) extract methods.

If we only add to_s to the top-level extract then it is possible for others pass non-strings to the more specific methods and cause an error: "others" here including future maintainers.

To put it more generally: only the more specific extract methods care that str is a string: extract doesn't need to know.

jbilbo commented 7 years ago

After discussing this with @mudge, let's leave it like that and once we refactor it we can move the .to_s to the entry point. It's safer that way, just in case this refactor never happens or there're other changes before it happens 👍