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

Add linter for DOI type citation. #484

Merged
merged 4 commits into from May 24, 2016

Conversation

Projects
None yet
3 participants
@mvdbeek
Copy link
Member

mvdbeek commented May 20, 2016

I guess this is useful on its own, and may prove more valuable if we support plugging tool information from http://bio.tools into planemo tool_init.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented May 20, 2016

Very nice - I love this!

@mvdbeek

This comment has been minimized.

Copy link
Member Author

mvdbeek commented May 20, 2016

Thanks @jmchilton - seems like I created a false positive in the tests though

@mvdbeek mvdbeek force-pushed the mvdbeek:doi_linter branch from 63a3e58 to ecfb625 May 20, 2016

@@ -0,0 +1,31 @@
<tool id="copy" name="Copy Dataset" version="1.0">
<description>copies a dataset</description>

This comment has been minimized.

@jmchilton

jmchilton May 23, 2016

Member

The test framework just ensures all fail_ tools fail linting - just rename to invaild_doi or something and the failing test will work.

This comment has been minimized.

@mvdbeek

mvdbeek May 24, 2016

Author Member

Ahh, indeed, that did the trick, thanks!

for citation in list(element):
if "type" in citation.attrib:
if citation.attrib["type"] == "doi":
dois.append(citation.text)

This comment has been minimized.

@nsoranzo

nsoranzo May 23, 2016

Member

I'd substitute the previous 3 lines with:

            if citation.tag == 'citation' and citation.attrib.get('type', '') == 'doi':
                dois.append(citation.text)
elif r.status_code == 404:
lint_ctx.error("%s is not a valid DOI" % publication_id)
else:
lint_ctx.info("dx.doi returned unexpected status code %d" % r.status_code)

This comment has been minimized.

@nsoranzo

nsoranzo May 23, 2016

Member

lint_ctx.warn ?

r = requests.get(url)
if r.status_code == 200:
if publication_id != doiless_publication_id:
lint_ctx.error("%s is valid, but galaxy expects DOI without 'doi:' prefix" % publication_id)

This comment has been minimized.

@nsoranzo

nsoranzo May 23, 2016

Member

s/galaxy/Galaxy/

@mvdbeek mvdbeek force-pushed the mvdbeek:doi_linter branch from ecfb625 to 51449bd May 24, 2016

Address review comments
Improve parsing of doi citation tag from xml, use lint_ctx.warn in case of
unexpected response from dx.doi and uppercase Galaxy (thanks @nsoranzo).
@@ -64,3 +64,8 @@ def test_empty_cdata(self):
empty_cdata = os.path.join(TEST_TOOLS_DIR, "empty_cdata.xml")
lint_cmd = ["lint", "--skip", "citations,help", empty_cdata]
self._check_exit_code(lint_cmd, exit_code=0)

def test_lint_doi(self):
fail_doi = os.path.join(TEST_TOOLS_DIR, "invalid.xml_doi.xml")

This comment has been minimized.

@nsoranzo

nsoranzo May 24, 2016

Member

s/invalid.xml_doi.xml/invalid_doi.xml/ ?

@jmchilton jmchilton merged commit b4852bb into galaxyproject:master May 24, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented May 24, 2016

Awesome thanks!

@mvdbeek mvdbeek referenced this pull request May 25, 2016

Merged

Fix doi style citations. #768

r = requests.get(url)
if r.status_code == 200:
if publication_id != doiless_publication_id:
lint_ctx.error("%s is valid, but Galaxy expects DOI without 'doi:' prefix" % publication_id)

This comment has been minimized.

@nsoranzo

nsoranzo May 25, 2016

Member

Are you sure about this? @bgruening pointed out in galaxyproject/tools-iuc#768 (comment) that the initial 'doi:' seems to work fine.

This comment has been minimized.

@mvdbeek

mvdbeek May 25, 2016

Author Member

I just tested it again, the doi breaks the display of the reference with my terribly out-of-date planemo instance (September 2015 I believe), but newer galaxy can deal with the doi:. I guess we could tone this down to an info?

This comment has been minimized.

@nsoranzo

nsoranzo May 25, 2016

Member

Something like:

lint_ctx.warn("%s is valid, but old Galaxy versions expected DOI without 'doi:' prefix" % publication_id)

?

This comment has been minimized.

@mvdbeek

mvdbeek May 26, 2016

Author Member

Yes ... but given those old instances will disappear over time, maybe we can remove the warning altogether? Alternatively we should at least figure out how old is old :/

This comment has been minimized.

@nsoranzo

nsoranzo May 26, 2016

Member

Up to you or @jmchilton, I don't have a strong opinion on this.

This comment has been minimized.

@jmchilton

jmchilton May 26, 2016

Member

I have no clue what changed to enable those - I just rewalked through the code and I can't even figure out why it works at all with the leading doi: - I don't think it should. Anyway - I definitely consider a leading doi: a tool bug and on a purely stylistic point somewhat redundant given the citation type is "doi". I think warn is entirely appropriate here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment