con / tributors

Pay tribute to your contributors! A tool to automatically update contributor files.
https://con.github.io/tributors/
Apache License 2.0
13 stars 6 forks source link

tweaking orcid search #48

Closed vsoch closed 4 years ago

vsoch commented 4 years ago

This pull request will do the following:

This means that with this verison, you should be able to run with interactive mode:

$ tributors update zenodo --interactive

Signed-off-by: vsoch vsochat@stanford.edu

vsoch commented 4 years ago

@yarikoptic after this PR I want to suggest you add comments to #26 so we can be sure that the orcid endpoints are working - please provide a detailed list of the tests you would like to see, thanks!

vsoch commented 4 years ago

Okay I've added the tests that I could - for the other-names and unicode tests, they won't work because we get more than one result (and the user would need to interactively look through them).

yarikoptic commented 4 years ago

Why am I getting a single result, i.e. how your query is different?

$> curl --silent -X GET --header 'Accept: application/json' 'https://pub.orcid.org/v3.0/expanded-search?q=Horea+AND+Christian' | jq .
{
  "expanded-result": [
    {
      "orcid-id": "0000-0001-7037-2449",
      "given-names": "Horea-Ioan",
      "family-names": "Ioanas",
      "credit-name": null,
      "other-name": [
        "Horea Christian"
      ],
      "email": [
        "ioanas@biomed.ee.ethz.ch"
      ],
      "institution-name": [
        "ETH",
        "Max-Planck-Institut für medizinische Forschung",
        "Saint Petersburg State University",
        "University of Heidelberg",
        "University of Oxford",
        "UniversitätsKlinikum Heidelberg"
      ]
    }
  ],
  "num-found": 1
}
vsoch commented 4 years ago

Your search doesn't have qualifiers like given-name, other-name, etc. Are you not using any qualifiers for any of your tests?

vsoch commented 4 years ago

that does the trick! Removing all parameters I get one result for the unicode and other name search. Did not expect that!

vsoch commented 4 years ago

@yarikoptic I think this is ready for merge, do you agree? The other issue with respect to caching is out of scope here.

vsoch commented 4 years ago

@yarikoptic I think the current changes are substantially better and we should merge and then discuss these nits further - if you feel strongly about any of the remaining points you can open up a separate PR to discuss.

yarikoptic commented 4 years ago

it is better indeed! I will just keep providing comments here. If you feel strongly about ignoring them - merge when you are satisfied. I will not have time for separate PRs and discussions. Sorry.

vsoch commented 4 years ago

Thanks @yarikoptic ! I don't feel empowered to make the changes that you envision, so I'm going to say perfect is the enemy of the good and get these (very good) changes in.