Skip to content
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

Show search by DOI in oa_snowball() examples #260

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

yjunechoe
Copy link
Collaborator

Ref: #259

(the package .Rd file got updated from running document() and reflects work of a prev PR)

@rkrug
Copy link

rkrug commented Jul 3, 2024

I would suggest to make doi an argument of the function to make clear that either the OA id or the doi can be used to do the snowball search. Now it is somehow hidden in the oa_snowball() call.

@yjunechoe
Copy link
Collaborator Author

It's as hidden in oa_snowball() as it is in oa_fetch() and everywhere else in {openalexR}, since doi is technically not an identifier for an OpenAlex object but a filter, like author.orcid, ids.mag, etc. Since it touches on other aspects of the package design, promoting doi as its own argument might need to be discussed in a separate issue.

IMO I think this is just a documentation problem - might be worth revisiting #30 while we're at it, to make it clear that oa_fetch() and oa_snowball() share the ... (namely, the query filters)

@rkrug
Copy link

rkrug commented Jul 3, 2024

Technically correct. But looking from the user perspective, who knows DOIs, and is not really aware of the inner workings (and does not have to be) of OpenAlex (and also openalexR), to see a DOI as an argument in oa_snowball() and oa_fetch() (as you mention it - did not cross my mind - but it is the same) would make this much easier to use and to start using it. The help file can specify the shortcomings of using DOIs.

@yjunechoe
Copy link
Collaborator Author

yjunechoe commented Jul 3, 2024

I think it's great that it didn't cross your mind for oa_fetch(): it means the documentation was sufficient but we simply fell short on doing the same for oa_snowball()!

I do see your point about the convenience of having doi "right there" in the function signature to see, so I'll keep this open for a bit for others to comment, but do note that the ... issue will still persist in oa_snowball() anyways, since we want to keep the design of forwarding ... into oa_fetch(), which is the main workhorse function.

In other words, even if we promote doi into its own argument, only oa_fetch() will get that in its function signature, and oa_snowball() will still "hide" it in the dots.

@rkrug
Copy link

rkrug commented Jul 3, 2024 via email

@yjunechoe
Copy link
Collaborator Author

yjunechoe commented Jul 3, 2024

Thanks, I see the point about "one starts with the papers first". Though IMO the best practice is to convert to OpenAlex IDs first before snowballing (the way you've been doing it already!), since some papers might not exist / might not be discoverable from DOI in OpenAlex, - that should be an input sanitization issue you ideally resolve before you move forward with a snowball.

Anyways, I'm more concerned about what this means for the broader design. Here's my thought process.

Q: Leave oa_snowball() aside for the moment - is the way users already search papers via doi in oa_fetch() not clear?

A:1 If it is clear, then no need to change anything about how oa_snowball() operates. We just need to make it clear to users that oa_snowball() work the same with oa_fetch() in terms of search filters. We maintain a single mental model about filters, and avoid any confusion by e.g., having oa_snowball() have an explicit doi argument, but oa_fetch() not (or, on the other extreme, committing to all current and future functions powered by oa_fetch() to redundantly specify a doi argument to meet that expectation)

A2: If it's not clear, we need to revisit the way we introduce ... and query filters entirely, and change the function signature for oa_fetch() as a last resort, as it introduces the most change. After oa_fetch() business is settled, we then revisit whether oa_snowball(), essentially a wrapper to oa_fetch(), should continue to pass the ... or take an explicit doi argument there as well.

Copy link
Collaborator

@trangdata trangdata left a comment

Choose a reason for hiding this comment

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

Thank you, June! 💯 I forgot to rerun document() after editing DESCRIPTION.

@trangdata
Copy link
Collaborator

@rkrug I don't support promoting doi as its own argument because other arguments like author.orcid would need the same treatment. I hope the new oa_snowball examples June added are clear enough.

A2: If it's not clear, we need to revisit the way we introduce ... and query filters entirely, and change the function signature for oa_fetch() as a last resort, as it introduces the most change. After oa_fetch() business is settled, we then revisit whether oa_snowball(), essentially a wrapper to oa_fetch(), should continue to pass the ... or take an explicit doi argument there as well.

I agree with this point, but we should also keep in mind that this would be a major refactoring that will likely introduce new bugs. We currently have filter = list(...) as an alternative place for putting all the filters. However, I have really enjoyed putting the filters as direct arguments to oa_fetch via the ellipses.

Let's go ahead and merge this PR and table this discussion for now. Refactoring will need to be in a different issue.

@rkrug
Copy link

rkrug commented Jul 3, 2024 via email

@yjunechoe
Copy link
Collaborator Author

Thanks both for the helpful discussion! Let's move what we discussed into a longer-form vignette in a separate issue/PR. I'll go ahead and merge this as it addresses #259 (a Q of whether doi search is possible at all)

@yjunechoe yjunechoe merged commit 8bd7dd2 into ropensci:main Jul 3, 2024
7 checks passed
@rkrug
Copy link

rkrug commented Jul 3, 2024

Let's go ahead and merge this PR and table this discussion for now. Refactoring will need to be in a different issue.

Agreed. Not only refactoring, but also other points raised (e.g. httr2 transition). If you could please start the discussion I would be happy to contribute to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Do snowball search also from DOI and not only from works id
3 participants