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

A fix to processing entries for zenodo, print ORCID URL in interactive mode, perform some internal code refactorings #78

Merged
merged 9 commits into from
Jun 7, 2023

Conversation

yarikoptic
Copy link
Member

One commit addresses #77 (has proper Closes). Individual commits have description on the changes so best to review one commit at a time.

So instead of

    INFO:github:Alejandro de la Vega: found more than 1 result, run with --interactive mode to select.
    INFO:github:Alejandro de la Vega: found more than 1 result, run with --interactive mode to select.
    INFO:github:Alejandro de la Vega: found more than 1 result, run with --interactive mode to select.

we get

	INFO:github:Alejandro de la Vega: found more than 1 (186) result for ORCID search by name, run with --interactive mode to select.
	INFO:github:Alejandro de la Vega: found more than 1 (1000) result for ORCID search by name without middle, run with --interactive mode to select.
	INFO:github:Alejandro de la Vega: found more than 1 (1000) result for ORCID search full name, run with --interactive mode to select.

(disregard that there is no middle name here... separate issue)
I think it does not change any result but IMHO should be more Kosher
…: just use min)

Without this fix I could not enter the last choice index
It now should work also disregarding explicitly specified to be null or empty
ORCID entries or values for name or affiliation which people might explicitly
white out in a specific case. Before it was solely based on the presence of the
key and I think making it more value based would be more robust.

Also dictionary comprehension for lookup generation should be easier to read IMHO,
and the code simplifications/reduction of [][] lookups also should be making it
easier to read.

Also may be it is just an anecdotal evidence and just coincided, but with this code
we got one more affiliation assigned in datalad case right after prior run
http://github.com/datalad/datalad/commit/ffc008b67c0596c16dadd7daeb58204a010ece74
@yarikoptic yarikoptic requested a review from vsoch June 7, 2023 02:24
@@ -129,7 +130,7 @@ def get_orcid_token():
return orcid_token


def record_search(url, email, interactive=False):
def record_search(url, email, interactive=False, how=""):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can tell what "how" is based on reading below how the functions are using it - can we use a more descriptive label here, like search type, and document this in the docstring so it's clear the string is just for informational purposes and doesn't derive any logic?

Copy link
Member Author

Choose a reason for hiding this comment

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

done in c76a90a

@yarikoptic yarikoptic added the minor Increment the minor version when merged label Jun 7, 2023
Copy link
Collaborator

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

If you’ve tested this and are happy, LGTM. Last final tweak is to please bump the version and update the CHANGELOG.md with a description of your fixes. Thank you!

@yarikoptic yarikoptic changed the title Fixes and enhancements A fix to processing entries for zenodo, print ORCID URL in interactive mode, perform some internal code refactorings Jun 7, 2023
@yarikoptic
Copy link
Member Author

Last final tweak is to please bump the version and update the CHANGELOG.md with a description of your fixes.

I have merged #74, so hopefully it all would just get released when we merge it. I added minor label, so it should make it 0.1.0.

@vsoch
Copy link
Collaborator

vsoch commented Jun 7, 2023

Hmm that approach seems super complex compared to what I usually do, but if you are managing it I’m indifferent. LGTM merge and release as you like.

@yarikoptic
Copy link
Member Author

Hmm that approach seems super complex compared to what I usually do, but if you are managing it I’m indifferent. LGTM merge and release as you like.

Initial setup is indeed a bit "complex" but whenever it works - it can't be simpler, just

  • use PR descriptions worth a changelog
  • label PRs with labels minor, patch, documentation, internal respectively
  • add release label to the PR after which you would like to have release done. So I am adding release label now and merging -- let's see if setup works or needs a few more tweeks

@yarikoptic yarikoptic added the release Trigger a release label Jun 7, 2023
@yarikoptic yarikoptic merged commit df59bca into master Jun 7, 2023
@yarikoptic yarikoptic deleted the fixes branch June 7, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Trigger a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants