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

Entrez journal examples fixes #735 #736

Closed
wants to merge 8 commits into from
Closed

Entrez journal examples fixes #735 #736

wants to merge 8 commits into from

Conversation

vincentdavis
Copy link
Contributor

Entrez journal examples fixes #735
Fixed two (all) examples, still searches journals but finds them via the nlmcatalog.

@codecov-io
Copy link

Current coverage is 78.69%

Merging #736 into master will not affect coverage as of fe1ae7e

@@            master    #736   diff @@
======================================
  Files          315     315       
  Stmts        46590   46590       
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit          36663   36665     +2
  Partial          0       0       
+ Missed        9927    9925     -2

Review entire Coverage Diff as of fe1ae7e

Powered by Codecov. Updated on successful CI builds.

@@ -263,12 +265,11 @@ \section{ESummary: Retrieving summaries from primary IDs}
>>> Entrez.email = "A.N.Other@example.com" # Always tell NCBI who you are
>>> handle = Entrez.esummary(db="journals", id="30367")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this line be changed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am too dyslexic for this.

@@ -200,18 +200,20 @@ \section{ESearch: Searching the Entrez databases}

As a final example, let's get a list of computational journal titles:
\begin{verbatim}
>>> handle = Entrez.esearch(db="journals", term="computational")
print("{} computational Journals found".format(record["Count"]))
Copy link
Member

Choose a reason for hiding this comment

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

That should have three leading >>> signs and the expected result if you want it to be part of the "doctest".

However, since this appears later I think this is more likely an accident and this line should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, it needed to be removed.

@vincentdavis
Copy link
Contributor Author

@peterjc Would you like me to refactor this pull request to be more like the previous example?

@@ -200,18 +200,19 @@ \section{ESearch: Searching the Entrez databases}

As a final example, let's get a list of computational journal titles:
\begin{verbatim}
>>> handle = Entrez.esearch(db="journals", term="computational")
>>> handle = Entrez.esearch(db="nlmcatalog", term="computational[Journal]", , RetMax='20')
Copy link
Member

Choose a reason for hiding this comment

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

The extra comma in there is a glitch, and I will remove it...

Copy link
Member

Choose a reason for hiding this comment

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

Also it should be retmax not RetMax although it "works" and the case is carried forward to the URL.

@peterjc
Copy link
Member

peterjc commented Jan 15, 2016

I was going to merge this (using .format() since that seems to be the way Python as a whole is moving even if I personally don't like the style), but spotted a couple of minor issues. If you want to update those, or just verbally agree the changes and I can tweak it here before updating the master:

, , RetMax='20') --> , retmax=20) (also note should be able to use an integer not a string)

, term="[journal]", --> , (i.e. remove this argment)

@vincentdavis
Copy link
Contributor Author

, term="[journal]", --> , (i.e. remove this argment)
This was an attempt to restrict the search to journals, but that is not how this works. update coming soon.

peterjc pushed a commit that referenced this pull request Feb 1, 2016
@peterjc
Copy link
Member

peterjc commented Feb 1, 2016

Applied as squashed commit 05ee639 - thanks!

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.

Entrez Tutorial, "journals", is no longer in Entrez databases.
4 participants