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

fix bug where retmode is not passed down to make_entrez_query in entr… #121

Merged
merged 1 commit into from
Mar 5, 2018

Conversation

gmbecker
Copy link
Contributor

The retmode parameter is ignored in entrez_fetch, which causes xml to be returned when a genbank file was asked for. This breaks integration with genbankr. If you could push a narrow bugfix to cran for this soon I would appreciate it, as genbankr is going to continue to fail in the Bioc build system until either this is fixed or I disable the integration (which Id on't want to do, since I think it's a very powerful feature).

I went ahead and bumped the version in the DESCRIPTION file because I need to be able to put a versioned dependency in genbankr, but if you'd prefer those happen separately I can recreate the PR without that.

@dwinter
Copy link
Member

dwinter commented Feb 20, 2018

Thanks for this @gmbecker, and apologies for the bug. Let me check a few things and I will try to get this up on CRAN today

@dwinter
Copy link
Member

dwinter commented Feb 20, 2018

Hey @gmbecker , I will improve the way retmode and rettype are documented and used. But for now I think you will just need to specify both arguments (the NCBI is now returning xml for genbank by default). This works for me on the CRAN version

cat(entrez_fetch(db='nuccore', rettype='gb',retmode='text', id=104))
LOCUS       CAA29094                 357 aa            linear   MAM 14-JUL-2016
DEFINITION  beta-subunit, partial [Bos taurus].
ACCESSION   CAA29094
VERSION     CAA29094.1
DBSOURCE    embl accession X05605.1
KEYWORDS    .
SOURCE      Bos taurus (cattle)
...

@gmbecker
Copy link
Contributor Author

gmbecker commented Feb 20, 2018 via email

@dwinter
Copy link
Member

dwinter commented Feb 21, 2018

Hi @gmbecker,

Seems like a few behaviours have changed recently at NCBI (so even with this fix some tests fail). Making a complete fix for this will need a few tweaks, and I would like to update the docs to make the retmode/rettypes a bit clearer. That probably means a CRAN release is a couple of days away.

In the meantime, I have merged the relevant bits of this PR into the develop branch. If you have users being struck by this you can get them to install the dev branch:

devtools::install_github("ropensci/rentrez", ref = "develop")

Thanks again for pointing this our and finding the solution. Will let you know when a release goes to CRAN.

@dwinter
Copy link
Member

dwinter commented Feb 27, 2018

Hi @gmbecker , sorry for the delay in getting a fix to this on CRAN.

The develop branch now was a complete fix for this with updates to tests and documentation. Can you confirm the version on the development branch works with genbankr? If so I can get it on CRAN and you'll stop being bugged by the Bioconductor testing platform :)

@gmbecker
Copy link
Contributor Author

@dwinter Looks looks good to go, vignette and tests now passing for release version of genbankr (they weren't before).

Can you confirm that 1.2.1 is the version number that will be used when submitting to CRAN? if so I'll push a versioned dep into the DESCRIPTION file for release and devel versions of genbankr and we should be good.

@dwinter
Copy link
Member

dwinter commented Feb 28, 2018

Hi @gmbecker. Yup, 1.2.1 uploaded to CRAN now, will hopefully be available in the next little while.

@dwinter dwinter merged commit 1bf7f84 into ropensci:master Mar 5, 2018
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.

2 participants